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

Temporarily disable local execution deduplication for Java compilation actions #23307

Closed

Conversation

fmeum
Copy link
Collaborator

@fmeum fmeum commented Aug 14, 2024

The deduplication action is prone to race conditions with actions that modify spawn contents after execution, which currently only Java compilation actions do.

It's still useful and expected to be functional for Starlark, C++ and JavaResourceJar actions.

Work towards #23288

@fmeum fmeum requested a review from a team as a code owner August 14, 2024 19:01
@github-actions github-actions bot added team-Remote-Exec Issues and PRs for the Execution (Remote) team awaiting-review PR is awaiting review from an assigned reviewer labels Aug 14, 2024
@fmeum fmeum requested a review from tjgq August 14, 2024 19:03
The deduplication action is prone to race conditions with actions that modify spawn contents after execution, which currently only Java compilation actions do.
@fmeum fmeum force-pushed the execution-deduplication-for-javac branch from 94e1850 to 51bd64d Compare August 14, 2024 19:27
@fmeum fmeum changed the title Disable local execution deduplication for Java compilation actions Temporarily disable local execution deduplication for Java compilation actions Aug 14, 2024
@fmeum
Copy link
Collaborator Author

fmeum commented Aug 20, 2024

@bazel-io fork 7.4.0

@tjgq tjgq added awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally team-Local-Exec Issues and PRs for the Execution (Local) team and removed awaiting-review PR is awaiting review from an assigned reviewer team-Remote-Exec Issues and PRs for the Execution (Remote) team labels Aug 21, 2024
@fmeum
Copy link
Collaborator Author

fmeum commented Aug 21, 2024

@tjgq In case you haven't merged this yet, we can drop it. I have a fix in #23382.

@tjgq
Copy link
Contributor

tjgq commented Aug 21, 2024

#23382 is exactly what I had in mind for async uploading :) I will review it tomorrow (today I'm traveling) but we can drop this one.

@tjgq tjgq removed the awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally label Aug 21, 2024
@tjgq tjgq closed this Aug 21, 2024
@fmeum fmeum deleted the execution-deduplication-for-javac branch August 21, 2024 15:04
copybara-service bot pushed a commit that referenced this pull request Aug 27, 2024
When an action may modify a spawn's outputs after execution, the upload of outputs to the cache and reuse for deduplicated actions need to happen synchronously directly after spawn execution to avoid a race.

This commit implements this for cache uploads by marking all actions with this property and simply disabling async upload for all spawns executed by such actions.

For output reuse, all executions deduplicated against the first one register atomically upon deduplication and cause the cache upload to wait for all of them to complete reuse.

Fixes #22501
Fixes #23288
Work towards #21578
Closes #23307 (no longer needed)

Closes #23382.

PiperOrigin-RevId: 668101364
Change-Id: Ice75dbe14a7dd46e02ecb096d2b2a30940216356
fmeum added a commit to fmeum/bazel that referenced this pull request Sep 19, 2024
When an action may modify a spawn's outputs after execution, the upload of outputs to the cache and reuse for deduplicated actions need to happen synchronously directly after spawn execution to avoid a race.

This commit implements this for cache uploads by marking all actions with this property and simply disabling async upload for all spawns executed by such actions.

For output reuse, all executions deduplicated against the first one register atomically upon deduplication and cause the cache upload to wait for all of them to complete reuse.

Fixes bazelbuild#22501
Fixes bazelbuild#23288
Work towards bazelbuild#21578
Closes bazelbuild#23307 (no longer needed)

Closes bazelbuild#23382.

PiperOrigin-RevId: 668101364
Change-Id: Ice75dbe14a7dd46e02ecb096d2b2a30940216356
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
team-Local-Exec Issues and PRs for the Execution (Local) team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants