fix(builtin): make linker aspect run in constant time per target #3301
Conversation
Previously, for every transitive dependency of a node binary target, the module_mappings_aspect had a runtime quadratic in the number of mappings: It traverses the mappings of all direct dependencies in a loop and called _link_mapping in its body, which again traverses the mappings. With this commit, the LinkerPackageMappingInfo provider is restructured to hold depsets of mapping entries and node_modules root paths. In this way the aspect invocations for the individual targets never traverse the (partially constructed) mappings. Instead, write_node_modules_manifest flattens the depset only after the aspect has run. If this should ever become a performance bottleneck again, the flattening could be moved into the execution phase by letting a dedicated binary consume the depset wrappen in an Args object. In an experiment with a single medium-sized package.json and a Node binary depending on all targets in the corresponding npm repository, this commit reduces the analysis wall time spent in the aspect implementation from ~30s to ~70ms.
fmeum
force-pushed
the
improve-linker-performance
branch
from
4719783
to
312dca0
Compare
|
@devversion do you have any input regarding the subtle breaking change here, and whether you think it merits the extra effort and temporary complexity to make it non-breaking? |
There was a problem hiding this comment.
@alexeagle From my perspective, given the scenario of manual linker mappings being advanced anyway (as stated in the PR as well), there is nothing to worry about in terms of making it non-breaking (for Angular the API change should be trivial at first glance).
This PR looks amazing, can't wait to see it published 🎉 Thank you!
There was a problem hiding this comment.
I think this is fine to land and be in a minor release, we should just mention in the release notes in case someone other than Angular was using the non-public API.
I'll let @gregmagolan do the approve since he's most familiar with this code.
| package_path: The root path of the node_modules under which this package should be linked. | ||
| link_path: The exec path under which the package is available. | ||
|
|
||
| Note: The depset may contain multiple entries per (package_name, package_path) pair. |
PR Checklist
PR Type
What is the current behavior?
Issue Number: #3302
What is the new behavior?
With this commit, the
LinkerPackageMappingInfoprovider is restructured to hold depsets of mapping entries and node_modules root paths. In this way the aspect invocations for the individual targets never traverse the(partially constructed) mappings.
Instead,
write_node_modules_manifestflattens the depset only after the aspect has run. If this should ever become a performance bottleneck again, the flattening could be moved into the execution phase by letting a dedicated binary consume the depset wrapped in an Args object.In an experiment with a single medium-sized package.json and a Node binary depending on all targets in the corresponding npm repository, this commit reduces the analysis wall time spent in the aspect implementation from ~30s to ~70ms.
Does this PR introduce a breaking change?
Dependencies providing their own
LinkerPackageMappingInfo(an advanced use case introduced by #2941) have to adapt to the structure of the provider. However, if that kind of breaking change is considered intolerable, it would be possible to add a translation layer between the old and new format.Other information