[WIP] Prioritize nodes with more waiting parents when adding new candidates #4155
Conversation
| for p, subtract in parents.items(): | ||
| p.ref_count = p.ref_count - subtract | ||
| if T: T.write(self.trace_message('Task.postprocess()', | ||
| p, | ||
| 'adjusted parent ref count')) | ||
| if p.ref_count == 0: | ||
| self.tm.candidates.append(p) | ||
| new_candidates.append(p) | ||
| self.tm.candidates.extend(sorted(new_candidates, key = lambda c: len(c.waiting_parents))) |
There was a problem hiding this comment.
Wouldn't it be better to append, then sort the whole array by # of parents?
There was a problem hiding this comment.
The candidates list can be very long. I don't think I'd want to pay to re-sort it each time we completed a node. The goal of this change is just to ensure that if post-processing a new node unblocks several others that we stage the newly unblocked node that will itself unblock the most work at the top of the candidates stack.
We could investigate whether using a more sophisticated data structure could be used here, but I'd want to do that as a separate PR. At minimum, it would be important to consider what effects that might have on Random. The current approach implements Random by randomizing the order in which things are added to the candidates list, but if that list is something like a priority queue where "number of things unblocked" is the prioritization, that'd mean that the insertion order doesn't matter. There is also the issue that the "number of things unblocked" is adjusted dynamically, since we may not yet have evaluated all the candidates that feed into that metric.
|
Let's hang back on merging this. I do think there is something to be done along these lines, but I 1) agree that this is at best a partial implementation because it doesn't globally prioritize, and 2) I haven't been able to demonstrate the increased build throughput that I hoped it would provide. |
mwichmann
changed the title
|
I've added a WIP to the title to that end |
|
@acmorrow - is this PR still valid/useful/ updatable with other changes already merged? |
|
I still think there is something to this idea, since, given the option, it always seems better to act on nodes with lots of waiting parents preferentially, but without a meaningful performance testing environment for SCons it is hard to show whether it actually improves throughput. |
Contributor Checklist:
CHANGES.txt(and read theREADME.rst)