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

Drop experimental_multi_threaded_digest #14034

Closed
meisterT opened this issue Sep 24, 2021 · 6 comments
Closed

Drop experimental_multi_threaded_digest #14034

meisterT opened this issue Sep 24, 2021 · 6 comments
Assignees
Labels
team-Local-Exec Issues and PRs for the Execution (Local) team type: process untriaged

Comments

@meisterT
Copy link
Member

This options defaults to true since a while and should just be removed, likely with the whole SsdModule.

@meisterT meisterT self-assigned this Sep 24, 2021
@oquenchil oquenchil added team-Local-Exec Issues and PRs for the Execution (Local) team type: process untriaged labels Sep 27, 2021
@bjacklyn
Copy link

bjacklyn commented Nov 2, 2021

@meisterT -- checking cached actions now takes 5-6x longer in my local builds after 6ac6954224b2b74c18d3218dfa299424cbc944fb was merged, that is what used to take 30 seconds for bazel to determine there was nothing to do in an already fully cached build, it now takes 2 mins 30 seconds

@bjacklyn
Copy link

bjacklyn commented Nov 2, 2021

There used to be a check against this boolean which I assume was defaulting to true:

-  private static final AtomicBoolean MULTI_THREADED_DIGEST = new AtomicBoolean(false);
...
-    if (fileSize > MULTI_THREADED_DIGEST_MAX_FILE_SIZE && !MULTI_THREADED_DIGEST.get()) {
+    if (fileSize > MULTI_THREADED_DIGEST_MAX_FILE_SIZE) {
       // We'll have to read file content in order to calculate the digest.
       // We avoid overlapping this process for multiple large files, as
       // seeking back and forth between them will result in an overall loss of
       // throughput.
       digest = getDigestInExclusiveMode(path);
     } else {
-      digest = getDigestInternal(path);
+      digest = path.getDigest();
     }

But now it always takes the slow path. I revived the fast checking cached actions with this:

-    if (fileSize > MULTI_THREADED_DIGEST_MAX_FILE_SIZE) {
+    if (fileSize > MULTI_THREADED_DIGEST_MAX_FILE_SIZE && !true) {

@meisterT
Copy link
Member Author

meisterT commented Nov 3, 2021

Ouch, that's definitely a mistake. I'm OOO this week but feel free to send a PR to fix the behavior.

cc @Wyverald this is a release blocker

@Wyverald Wyverald added this to the Bazel 5.0 Release Blockers milestone Nov 3, 2021
@Wyverald Wyverald reopened this Nov 3, 2021
@Wyverald
Copy link
Member

Wyverald commented Nov 3, 2021

Cherrypicks are still open.

bjacklyn pushed a commit to bjacklyn/bazel that referenced this issue Nov 5, 2021
…been removed

There used to be a cli flag --experimental_multi_threaded_digest which defaulted to true
and therefore would not compute the file digest in exclusive mode unless overridden to false.
When that flag was removed this code was accidentally left behind which instead causes bazel to always
compute file digests in exclusive mode for all files larger than MULTI_THREADED_DIGEST_MAX_FILE_SIZE.

This causes bazel to take 5-6x longer to 'check cached actions' in my org's builds,
specifically increasing the time from 30 secs to 2 mins 30 secs for fully cached no-op builds.

Fixes bazelbuild#14034
@brentleyjones
Copy link
Contributor

Should be reopened as it's still blocking 5.0 (needs to be cherry-picked).

@Wyverald Wyverald reopened this Nov 5, 2021
meisterT pushed a commit to meisterT/bazel that referenced this issue Nov 8, 2021
…been removed

There used to be a cli flag --experimental_multi_threaded_digest which defaulted to true
and therefore would not compute the file digest in exclusive mode unless overridden to false.
When that flag was removed this code was accidentally left behind which instead causes bazel to always
compute file digests in exclusive mode for all files larger than MULTI_THREADED_DIGEST_MAX_FILE_SIZE.

This causes bazel to take 5-6x longer to 'check cached actions' in my org's builds,
specifically increasing the time from 30 secs to 2 mins 30 secs for fully cached no-op builds.

Fixes bazelbuild#14034

Closes bazelbuild#14231.

PiperOrigin-RevId: 407790990
(cherry picked from commit 77a002c)
Wyverald pushed a commit that referenced this issue Nov 8, 2021
#14242)

…been removed

There used to be a cli flag --experimental_multi_threaded_digest which defaulted to true
and therefore would not compute the file digest in exclusive mode unless overridden to false.
When that flag was removed this code was accidentally left behind which instead causes bazel to always
compute file digests in exclusive mode for all files larger than MULTI_THREADED_DIGEST_MAX_FILE_SIZE.

This causes bazel to take 5-6x longer to 'check cached actions' in my org's builds,
specifically increasing the time from 30 secs to 2 mins 30 secs for fully cached no-op builds.

Fixes #14034

Closes #14231.

PiperOrigin-RevId: 407790990
(cherry picked from commit 77a002c)

Co-authored-by: Brandon Jacklyn <brandonjacklyn@gmail.com>
@Wyverald
Copy link
Member

Wyverald commented Nov 8, 2021

Cherrypicked.

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 type: process untriaged
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants