refactor: defer zip manifest building to execution phase to improve analysis phase performance#3381
Conversation
|
As mentioned in the other PR comment: we can definitely accept PR until the questions about using an external tool are worked out. Overall, LGTM. Because of the profile results in the other thread showing format() (as called by map_zip_runfiles) was a significant chunk of the time, changing to |
tobyh-canva
changed the title
py_binary and py_test rules
rickeylev
changed the title
py_binary and py_test rules
|
For posterity: Before this change, about 462 seconds is spent in py_binary related to zip file manifest building. With this change, the profile shows about 46 seconds spent in py_binary related to zip file manifest building. 
This profile is this change, but before changing to use |
|
Ok, cleaned this up, switched it to use @tobyh-canva If you have opportunity, could you run another profile? I'm interested to see how much of the 23 seconds of format() overhead in building the path strings is gone. |
|
Running a profile right now, thanks heaps! |
|
Here's the before/after profile on our repo with these changes: So removing the |
Looking at the investigation in bazel-contrib#3381, it seems that we are calling the startswith many times and I wanted to see if it would be possible to optimize how it is done. I also realized that no matter what target we have, we will be calling the function once with a `__init__.py` path and we can inline this case as a separate if statement checking for equality instead, which Starlark optimizer should understand better. Before this PR for every executable target we would go through the `legacy_external_runfiles and "__init__.py".startswith("external")` and this PR eliminates this. Related to bazel-contrib#3380 and bazel-contrib#3381
) Looking at the investigation in #3380, it seems that we are calling the startswith many times and I wanted to see if it would be possible to optimize how it is done. I also realized that no matter what target we have, we will be calling the function once with a `__init__.py` path and we can inline this case as a separate if statement checking for equality instead, which Starlark optimizer should understand better. Before this PR for every executable target we would go through the `legacy_external_runfiles and "__init__.py".startswith("external")` and this PR eliminates this. Related to #3380 and #3381
2 participants
When py_binary/py_test were being built, they were flattening the runfiles
depsets at analysis time in order to create the zip file mapping manifest for
their implicit zipapp outputs. This flattening was necessary because they had
to filter out the original main executable from the runfiles that didn't belong
in the zipapp. This flattening is expensive for large builds, in some cases
adding over 400 seconds of time and significant memory overhead.
To fix, have the zip file manifest use the
runfiles_with_exeobject, which isthe runfiles, but pre-filtered for the files zip building doesn't want. This
then allows passing the depsets directly to
Args.add_alland using map_eachto transform them.
Additionally, pass
runfiles.empty_filenamesusing a lambda. Accessing thatattribute implicitly flattens the runfiles.
Finally, because the original profiles indicated
str.format()was a non-trivialamount of time (46 seconds / 15% of build time), switch to using
+instead.This is a more incremental alternative to #3380 which achieves most of the
same optimization with only Starlark changes, as opposed to introducing an
external script written in C++.
Profile of a large build, which shows a Starlark CPU profile. It shows an overall build
time of 305 seconds. 46 seconds (15%) are spent in
map_zip_runfiles, half of whichis in
str.startswith()and the other half instr.format().