Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] Prioritize nodes with more waiting parents when adding new candidates #4155

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

[WIP] Prioritize nodes with more waiting parents when adding new candidates #4155

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions CHANGES.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -152,6 +152,10 @@ RELEASE VERSION/DATE TO BE FILLED IN LATER
- Added MSVC_USE_SCRIPT_ARGS variable to pass arguments to MSVC_USE_SCRIPT.
- Added Configure.CheckMember() checker to check if struct/class has the specified member.

From Andrew Morrow:
- Prioritize nodes with more waiting parents when adding new candidate nodes.
NOTE: This may change build order and expose missing dependency edges

RELEASE 4.3.0 - Tue, 16 Nov 2021 18:12:46 -0700

From Jacob Cassagnol:
Expand Down
1 change: 1 addition & 0 deletions RELEASE.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,7 @@ CHANGED/ENHANCED EXISTING FUNCTIONALITY
require delayed expansion to be enabled which is currently not supported and is
typically not enabled by default on the host system. The batch files may also require
environment variables that are not included by default in the msvc environment.
- Prioritize nodes with more waiting parents when adding new candidate nodes.

FIXES
-----
Expand Down
4 changes: 3 additions & 1 deletion SCons/Taskmaster.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -471,13 +471,15 @@ def postprocess(self):
if p.ref_count == 0:
self.tm.candidates.append(p)

new_candidates = []
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)))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't it be better to append, then sort the whole array by # of parents?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


for t in targets:
t.postprocess()
Expand Down