feat: use primary-dependency attribute to find primary dependency of a task
#736
Conversation
bhearsum
added
the
BREAKING CHANGE
bhearsum
force-pushed
the
push-lqokptzsytmy
branch
4 times, most recently
from
f02dfbe to
0474896
Compare
ahal
left a comment
ahal
left a comment
There was a problem hiding this comment.
Thanks makes sense!
I think Abhishek might also have a backwards incompatible change coming soon, so it might be worth waiting for his patch and releasing them together.
| ) | ||
|
|
||
| primary_dep = [dep for dep in group if dep.kind == primary_kind][0] | ||
| new_task["attributes"]["primary-dependency"] = primary_dep |
There was a problem hiding this comment.
Should we anticipate issues from storing an object as an attribute rather than a string? Is there precedent for this?
There was a problem hiding this comment.
There was precedent for it until it was removed in https://phabricator.services.mozilla.com/D193079 😆
I admit it's a bit weird. Maybe we can just store the primary dependency label, and look it up by that instead, come to think of it?
There was a problem hiding this comment.
Yes, that works, and is much less weird and memory consuming! It arguably makes this a non-breaking change now, too? (I guess there's other ways that primary-kind-dependency might be set other than from_deps though...)
|
I am not working on the duplicate dependency patch currently so it would fine to land this |
bhearsum
force-pushed
the
push-lqokptzsytmy
branch
from
0474896 to
9fee389
Compare
…f a task Storing this on `from_deps` tasks allows us to avoid iterating over potentially all the task dependencies when finding it later, which can add up to a lot of time in larger graphs.
bhearsum
force-pushed
the
push-lqokptzsytmy
branch
from
9fee389 to
baf96e2
Compare
|
Do you want another look at this, @ahal ? |
I'll take silence as a no! If there's anything you'd like followed-up on, I can do so after the fact. |
3 participants
Storing this on
from_depstasks allows us to avoid iterating over potentially all the task dependencies when finding it later, which can add up to a lot of time in larger graphs.In a profile I ran for gecko taskgraph,
get_dependencies(viaget_primary_dependency) was the second hottest function. With this patch it moves down to 10th place, and takes 1/4 of the amount of time it used to.