-
Notifications
You must be signed in to change notification settings - Fork 4k
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.
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
support persistent worker for aar extractors #18496
Conversation
d4379ae
to
aa06c30
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few general comments:
- If we're adding a new execution mode for aar extraction, we need to add a test for it. Please see how other tools (i.e. busybox, desguar, dex) test their worker/multiplex worker functionality. I think a lot of those tests are in src/test/bazel/android/android_integration_test.sh.
- Please add more comments describing what your changes are doing, since behavior around execution strategy is critical functionality, and we want the logic to be as clear as possible.
- In general the json_worker_wrapper.py file seems quite verbose and low-level for a python program. I would imagine that a lot of this functionality can be refactored to be more pythonic.
src/main/java/com/google/devtools/build/lib/rules/android/AarImport.java
Show resolved
Hide resolved
src/main/java/com/google/devtools/build/lib/rules/android/AarImport.java
Outdated
Show resolved
Hide resolved
src/main/java/com/google/devtools/build/lib/rules/android/AarImport.java
Outdated
Show resolved
Hide resolved
src/main/java/com/google/devtools/build/lib/rules/android/AarImport.java
Outdated
Show resolved
Hide resolved
I'm also curious about what the cumulative time savings of all aar extraction actions is for your change. Your screenshot shows a 758 -> 15 ms wall time reduction, which is indeed impressive, but I think that's just for a single action. |
It saves nearly 100 seconds for the first-time compilation without cache. It had nearly 2000 extractor actions executed for my project, each of which saved nearly 500ms. I run it on a 10-core CPU, 500ms * 2000 / 10 ≈ 100s |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some minor changes requested for this iteration. Looking much better.
When will this PR be merged? Do I need to do anything else? :) @ted-xie |
Hi @SupSaiYaJin, I will update you once it's merged. Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @SupSaiYaJin, we ran into a couple linting issues while importing these. Could you take a look at the comments and adress them? Thank you!
@SupSaiYaJin Stepping back a little, I'm curious what are your thoughts on using a faster unzip implementation, versus implementing worker support for AAR extraction. We could use this as an opportunity to refactor AAR extraction in general to use a different zip inflate implementation, versus adding significant complexity (new flag, new execution strategy) to AAR extraction which in general is not supposed to be the bottleneck on build workflows. |
@ted-xie What would this implementation look like? If it also needs to start a new process to run, I think it still needs to run in worker mode. |
74ee50a
to
c7b5e32
Compare
Sorry, I missed the notification for this PR.
Jar/aar extraction is basically just unzip; if you choose a faster unzip executable, the combined process call + execution time may be less than the performance gain from worker mode.
Much of the current performance penalty for this action comes from having to spin up a java process, i.e. the entire JVM needs to come alive. The same limitation does not exist for unzip tools written in a non-runtime language. |
@ted-xie We can use worker for now and do it in the future, I think it's a good idea :). |
Hi @SupSaiYaJin , Could you please update the latest changes from master into the branch as it is awaiting to merge. Thanks! |
c7b5e32
to
f6780ac
Compare
@sgowroji done |
f6780ac
to
3f46298
Compare
Unblocks lint-related problems in #18496. PiperOrigin-RevId: 553886150 Change-Id: Ie7f5d369def2b5556fefe24e78e7a57d94921766
3f46298
to
c51dc0a
Compare
@ted-xie done |
@ted-xie Do you use any instant messaging apps? I would like to have a more in-depth conversation with you. Could we add each other as friends? |
This PR supports persistent worker for Android aar extractors, it'll significantly improve the aar extractors performance especially in the first time compilation without cache.
It's not enable by default, can use flag
build --experimental_persistent_aar_extractor=true
to enable it.using worker:
not using worker: