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.

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

Make d8 dex builder compatible with persistent worker mode #10241

Closed
gyias opened this issue Nov 14, 2019 · 11 comments
Closed

Make d8 dex builder compatible with persistent worker mode #10241

gyias opened this issue Nov 14, 2019 · 11 comments
Assignees
Labels
P2 We'll consider working on this in future. (Assignee optional) team-Android Issues for Android team type: feature request

Comments

@gyias
Copy link
Member

gyias commented Nov 14, 2019

Description of the problem / feature request:

Make d8 dex builder compatible with the persistent worker mode.

Feature requests: what underlying problem are you trying to solve with this feature?

D8 dex builder doesn't support the local persistent worker mode, so users have to specify --nouse_workers_with_dexbuilder flag additionally. Even worse, failing to do so can lead to an obscure behavior that can be challenging to debug. Fix it so that the flag is not required.

@tokudu
Copy link

tokudu commented Mar 27, 2020

@gyias i was chatting with @nkoroste, and it seems that #10844 is no longer an issue. The only issue we are seeing is this one.

I would be happy to contribute if you point me the right direction. Thanks!

@jongerrish
Copy link
Contributor

@sgjesse - I noticed you've made a number of commits to add //src/tools/android/java/com/google/devtools/build/android/r8:DexBuilder. We are very interested in a persistent D8 Builder and D8 Desugaring - what's the status of this work?

@jongerrish
Copy link
Contributor

I tried changing //tools/android:dexbuilder to use //src/tools/android/java/com/google/devtools/build/android/r8:DexBuilder rather than :dexbuilder but get build failures when building the tool from our workspace due to missing dependencies. (com.android.tools.r8.CompatDxSupport etc). I don't see this class in any recent versions of d8 (https://r8.googlesource.com/r8/+/refs/heads/master/src/main/java/com/android/tools/r8)

Any pointers as we'd love to try this out.

@sgjesse
Copy link
Contributor

sgjesse commented Jul 28, 2020

The reason you do not find CompatDxSupport in the R8 repository any more is that it was moved into the bazel repository (https://github.com/bazelbuild/bazel/blob/a3a854dfc084e8e5115c524cc0030653f9df0778/src/tools/android/java/com/android/tools/r8/CompatDxSupport.java) but still in the com.android.tools.r8 package to allow access to R8 package private APIs.

The work I have been doing is moving blaze desugaring to use the D8 desugaring engine, and that is still WIP. Supporting persistent workers can be a side effect of this work, as this will also move the bazel specific code out of the R8 repository so that bazel support only uses the public D8/R8 API.

@jongerrish
Copy link
Contributor

@sgjesse - Great to hear, persistent worker support for D8 is something we're expecting to help improve our build times.

Is the plan to have Desugaring performed in a separate Action to Dexing or will they be combined into a single Action? I'm wondering if any thought has been given regarding the tradeoffs for performance (isn't it faster to do both in the same pass?) and whether any consideration has been given to making desugaring optional for certain rules, e.g: R classes / Nano protos etc don't need desugaring.

Really excited by this work, please let me know if there's anything we can do to test it out early, more than happy to give feedback, report issues etc.

@jongerrish
Copy link
Contributor

@sgjesse - Hi Soren, any update on this? Will it be available as an option in the next release of Bazel? We have a pretty big Android codebase at Snap - I'd love to give it a try and report back any issues.

@ahumesky ahumesky added P2 We'll consider working on this in future. (Assignee optional) type: feature request and removed untriaged labels Dec 3, 2020
@nkoroste
Copy link
Contributor

nkoroste commented Jul 21, 2021

Any updates on this? I see that dx is now removed from the latest version of build-tools

@nkoroste
Copy link
Contributor

might be related #14623

@Bencodes
Copy link
Contributor

#14623 landed so you can start passing --use_workers_with_dexbuilder if you are using CompatDexBuilder. We've seen some pretty impressive performance wins from having this enabled, so curious to see if everyone else sees similar wins.

bazel-io pushed a commit that referenced this issue May 23, 2022
Removing the TODO on `use_workers_with_dexbuilder` and flipped the flag back to true.

Related issue #10241

Closes #15320.

PiperOrigin-RevId: 450513994
@github-actions
Copy link

Thank you for contributing to the Bazel repository! This issue has been marked as stale since it has not had any activity in the last 1+ years. It will be closed in the next 14 days unless any other activity occurs or one of the following labels is added: "not stale", "awaiting-bazeler". Please reach out to the triage team (@bazelbuild/triage) if you think this issue is still relevant or you are interested in getting the issue resolved.

@github-actions github-actions bot added stale Issues or PRs that are stale (no activity for 30 days) and removed stale Issues or PRs that are stale (no activity for 30 days) labels Jun 27, 2023
@ahumesky
Copy link
Contributor

This has been implemented

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P2 We'll consider working on this in future. (Assignee optional) team-Android Issues for Android team type: feature request
Projects
None yet
Development

No branches or pull requests

8 participants