Skip to content

Only register .jdeps caching ActionContext when used#21290

Closed
fmeum wants to merge 2 commits intobazelbuild:masterfrom
fmeum:19872-insert-dependencies
Closed

Only register .jdeps caching ActionContext when used#21290
fmeum wants to merge 2 commits intobazelbuild:masterfrom
fmeum:19872-insert-dependencies

Conversation

@fmeum
Copy link
Copy Markdown
Collaborator

@fmeum fmeum commented Feb 11, 2024 •
edited
Loading

By registering JavaCompileActionContext (renamed to JdepsCachingActionContext in this change to better reflect its only purpose) only when using --java_classpath=bazel, the insertDependencies field of JavaHeaderCompileAction becomes obsolete. Combined with the existing 3 bytes of padding, this makes room for a new 4-byte field that will be used in a rollforward of f5d76b1.

Along the way, skip the unnecessary work of reading the .jdeps in JavaHeaderCompilerAction#afterExecute if neither using the .jdeps cache nor path mapping.

Work towards #21091

fmeum added 2 commits February 11, 2024 16:04
@fmeum
Only register .jdeps cache ActionContext when used
0905d4c 
By registering `JavaCompileActionContext` (renamed to
`JdepsCachingActionContext` in this change to better reflect its only
purpose) only when using `--java_classpath=bazel`, the
`insertDependencies` field of `JavaHeaderCompileAction` becomes
obsolete. Combined with the existing 3 bytes of padding, this makes room
for a new 4-byte field that will be used in a rollforward of
fd196bf.
@fmeum
Update JavaHeaderCompileAction.java
eb1b4ea
@fmeum fmeum changed the title Only register .jdeps cache ActionContext when used Only register .jdeps caching ActionContext when used Feb 11, 2024
@fmeum fmeum mentioned this pull request Feb 11, 2024
Closed
@fmeum
Copy link
Copy Markdown
Collaborator Author

fmeum commented Feb 11, 2024

Additional context is provided in #19872 (comment).

fmeum
fmeum commented Feb 11, 2024
View reviewed changes
src/main/java/com/google/devtools/build/lib/bazel/rules/BazelRulesModule.java
BuildRequest buildRequest) {
registryBuilder.register(JavaCompileActionContext.class, new JavaCompileActionContext());
JavaOptions javaOptions = env.getOptions().getOptions(JavaOptions.class);
if (javaOptions != null
Copy link
Copy Markdown
Collaborator Author

@fmeum fmeum Feb 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Blaze analogue of this setup would also have to gate adding the context behind this condition.

@fmeum fmeum requested a review from hvadehra February 11, 2024 17:05
@fmeum fmeum marked this pull request as ready for review February 11, 2024 17:05
@fmeum fmeum requested a review from lberki as a code owner February 11, 2024 17:05
@fmeum fmeum removed the request for review from lberki February 11, 2024 17:05
@github-actions github-actions bot added team-Rules-Java Issues for Java rules awaiting-review PR is awaiting review from an assigned reviewer labels Feb 11, 2024
@fmeum
Copy link
Copy Markdown
Collaborator Author

fmeum commented Feb 11, 2024

@bazel-io fork 7.1.0

@bazel-io bazel-io mentioned this pull request Feb 11, 2024
Closed
@fmeum
Copy link
Copy Markdown
Collaborator Author

fmeum commented Feb 11, 2024

@hvadehra Btw, does Google use all three modes of --java_classpath? I'm wondering whether it would make sense to flip the default for Bazel to bazel (from javabuilder).

@hvadehra hvadehra requested a review from gregestren February 12, 2024 12:07
@hvadehra
Copy link
Copy Markdown
Member

hvadehra commented Feb 12, 2024 •
edited
Loading

  1. I'm a little unsure about this change. IIUC this change tries to derive the value of insertDependencies from the presence of JavaCompileActionContext/JdepsCachingActionContext. But those need not be the same, given L374-377. So, after this change, we'd be populating the cache even in those cases where we turn off the bazel classpath mode. I don't know if this would matter in practice, but given that this check was explicitly introduced in 5f9e9f0, I'd like @gregestren to have a look as well.

  2. I haven't looked at the details of the regression discussed in Also path map transitive header jar paths with direct classpath optimization #19872 (comment) but looking at the numbers, I estimate the contribution of the extra field is ~5% of the total regression. This might actually be an acceptable number. I'd recommend tackling the rest first, and then adding this change on if necessary later. In any case, if there is a O(days) gap between merging this change and the rollforward, that latter may still be flagged as a regression since the baseline for measurement will include this change. So, having both ready to be merged together will make things simpler.

  3. The default for --java_classpath internally is bazel as well, and that's the mode used by almost all builds (a small number (0.007%) do use the javabuilder mode, but I'll have to investigate to see why). So flipping the default should be mostly a no-op internally and SGTM.

@fmeum
Copy link
Copy Markdown
Collaborator Author

fmeum commented Feb 12, 2024

Thanks for sharing more numbers, I agree that this particular change is less important in this case. I will look into the more pressing problem first.

Could you link to the lines you referred to as L374-377? I meant for this PR to register the context exactly when using bazel, which was the condition that insertDependencies encoded.

@hvadehra
Copy link
Copy Markdown
Member

hvadehra commented Feb 12, 2024

Could you link to the lines you referred to as L374-377

I'd meant to do that but forgot, sorry about that: https://github.com/bazelbuild/bazel/blob/master/src/main/java/com/google/devtools/build/lib/rules/java/JavaHeaderCompileAction.java#L373-L377

@fmeum
Copy link
Copy Markdown
Collaborator Author

fmeum commented Feb 12, 2024

Thanks, now I fully understand your comment. This logic does make it difficult to replace the field, assuming it is needed at all.

I will close this PR for now and think about how to prevent the large part of the regression.

I will also look into submitting a PR to flip the default classpath mode for Bazel.

@fmeum fmeum closed this Feb 12, 2024
@github-actions github-actions bot removed the awaiting-review PR is awaiting review from an assigned reviewer label Feb 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@hvadehra hvadehra Awaiting requested review from hvadehra

@gregestren gregestren Awaiting requested review from gregestren

Assignees

No one assigned

Labels

team-Rules-Java Issues for Java rules

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@fmeum @hvadehra