Skip to content

Fix and optimize Runfiles#fingerprint #18384

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.

Sign up for GitHub

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

Jump to bottom
Closed
fmeum wants to merge 3 commits into from
Closed

Fix and optimize Runfiles#fingerprint #18384

fmeum wants to merge 3 commits into from

Conversation

@fmeum
Copy link
Collaborator

@fmeum fmeum commented May 12, 2023 •
edited
Loading

The fingerprint did not include the conflict policy and some of the collections' sizes. It also didn't use the cache for fingerprints of NestedSets and instead always flattened the sets.

The new fingerprint method on EmptyFilesSupplier makes it possible to drop the call to Runfiles#getEmptyFilenames, which would still end up flattening the sets.

See https://groups.google.com/g/bazel-discuss/c/KrUg6ZPky80

@fmeum fmeum force-pushed the runfiles-fingerprint branch 2 times, most recently from dfcf2eb to 6103e8a Compare May 13, 2023 09:29
@fmeum fmeum changed the title Runfiles fingerprint Fix and optimize Runfiles#fingerprint May 13, 2023
@fmeum
Fix and optimize Runfiles#fingerprint
78a896e 
The fingerprint did not include the conflict policy and some of the
collection's sizes. It also didn't use the cache for fingerprints of
`NestedSet`s and instead always flattened the sets.

The new `fingerprint` method on `EmptyFilesSupplier` makes it possible
to drop the call to `Runfiles#getEmptyFilenames`, which would still end
up flattening the sets.
@fmeum fmeum force-pushed the runfiles-fingerprint branch from 6103e8a to 78a896e Compare May 13, 2023 09:34
@fmeum fmeum requested review from haxorz and lberki May 13, 2023 09:34
@fmeum fmeum marked this pull request as ready for review May 13, 2023 09:36
@fmeum fmeum requested review from a team, comius and rickeylev as code owners May 13, 2023 09:36
@fmeum fmeum requested review from sdtwigg and removed request for a team, comius, rickeylev and sdtwigg May 13, 2023 09:36
@github-actions github-actions bot added awaiting-review PR is awaiting review from an assigned reviewer team-Configurability platforms, toolchains, cquery, select(), config transitions team-Performance Issues for Performance teams team-Rules-Python Native rules for Python labels May 13, 2023
lberki
lberki reviewed May 15, 2023
View reviewed changes
Copy link
Contributor

@lberki lberki left a comment

Choose a reason for hiding this comment

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

Thanks! The only real issue I have is with InterruptedException handling; it's easy to get that wrong.

src/main/java/com/google/devtools/build/lib/rules/python/PythonUtils.java
*/
public static class GetInitPyFiles implements Runfiles.EmptyFilesSupplier {
private final Predicate<PathFragment> isPackageInit;
private final UUID guid;
Copy link
Contributor

@lberki lberki May 15, 2023

Choose a reason for hiding this comment

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

nit: mind adding a comment as to why the UUID is necessary here? It's a little surprising if one doesn't intimately know how Runfiles works.

Copy link
Collaborator Author

@fmeum fmeum May 16, 2023

Choose a reason for hiding this comment

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

I added a comment to the constructor argument. Should I also add it to the field?

Copy link
Contributor

@lberki lberki May 16, 2023

Choose a reason for hiding this comment

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

Meh, I won't make you jump through that particular hoop; it's not that big a deal.

haxorz
haxorz reviewed May 15, 2023
View reviewed changes
Copy link
Contributor

@haxorz haxorz left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Can you please include a link to your bazel-discuss@ thread in the commit message?

@fmeum fmeum requested review from haxorz and lberki May 16, 2023 10:08
lberki
lberki approved these changes May 16, 2023
View reviewed changes
Copy link
Contributor

@lberki lberki left a comment

Choose a reason for hiding this comment

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

Thanks for this change! Let's merge it.

@lberki lberki added awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally and removed awaiting-review PR is awaiting review from an assigned reviewer labels May 16, 2023
@fmeum fmeum mentioned this pull request May 19, 2023
Closed
@sgowroji sgowroji removed the awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally label May 24, 2023
@fmeum fmeum deleted the runfiles-fingerprint branch May 24, 2023 09:55
fweikert pushed a commit to fweikert/bazel that referenced this pull request May 25, 2023
@fmeum @fweikert
Fix and optimize Runfiles#fingerprint
36d4ecf 
The fingerprint did not include the conflict policy and some of the collections' sizes. It also didn't use the cache for fingerprints of `NestedSet`s and instead always flattened the sets.

The new `fingerprint` method on `EmptyFilesSupplier` makes it possible to drop the call to `Runfiles#getEmptyFilenames`, which would still end up flattening the sets.

See https://groups.google.com/g/bazel-discuss/c/KrUg6ZPky80

Closes bazelbuild#18384.

PiperOrigin-RevId: 534724771
Change-Id: I7b39a1fa2c7c5904b186cc2d343b2b6432b05ad4
@fmeum fmeum mentioned this pull request Jun 15, 2023
Closed
iancha1992 pushed a commit to iancha1992/bazel that referenced this pull request Jun 16, 2023
@fmeum @iancha1992
Fix and optimize Runfiles#fingerprint
56a6e01 
The fingerprint did not include the conflict policy and some of the collections' sizes. It also didn't use the cache for fingerprints of `NestedSet`s and instead always flattened the sets.

The new `fingerprint` method on `EmptyFilesSupplier` makes it possible to drop the call to `Runfiles#getEmptyFilenames`, which would still end up flattening the sets.

See https://groups.google.com/g/bazel-discuss/c/KrUg6ZPky80

Closes bazelbuild#18384.

PiperOrigin-RevId: 534724771
Change-Id: I7b39a1fa2c7c5904b186cc2d343b2b6432b05ad4
@iancha1992 iancha1992 mentioned this pull request Jun 16, 2023
Closed
iancha1992 pushed a commit to iancha1992/bazel that referenced this pull request Jun 16, 2023
@fmeum @iancha1992
Fix and optimize Runfiles#fingerprint
2d49b63 
The fingerprint did not include the conflict policy and some of the collections' sizes. It also didn't use the cache for fingerprints of `NestedSet`s and instead always flattened the sets.

The new `fingerprint` method on `EmptyFilesSupplier` makes it possible to drop the call to `Runfiles#getEmptyFilenames`, which would still end up flattening the sets.

See https://groups.google.com/g/bazel-discuss/c/KrUg6ZPky80

Closes bazelbuild#18384.

PiperOrigin-RevId: 534724771
Change-Id: I7b39a1fa2c7c5904b186cc2d343b2b6432b05ad4
iancha1992 pushed a commit to iancha1992/bazel that referenced this pull request Jun 22, 2023
@fmeum @iancha1992
Fix and optimize Runfiles#fingerprint
d63a6aa 
The fingerprint did not include the conflict policy and some of the collections' sizes. It also didn't use the cache for fingerprints of `NestedSet`s and instead always flattened the sets.

The new `fingerprint` method on `EmptyFilesSupplier` makes it possible to drop the call to `Runfiles#getEmptyFilenames`, which would still end up flattening the sets.

See https://groups.google.com/g/bazel-discuss/c/KrUg6ZPky80

Closes bazelbuild#18384.

PiperOrigin-RevId: 534724771
Change-Id: I7b39a1fa2c7c5904b186cc2d343b2b6432b05ad4
@iancha1992 iancha1992 mentioned this pull request Jun 22, 2023
Closed
iancha1992 pushed a commit to iancha1992/bazel that referenced this pull request Jun 22, 2023
@fmeum @iancha1992
Fix and optimize Runfiles#fingerprint
2180160 
The fingerprint did not include the conflict policy and some of the collections' sizes. It also didn't use the cache for fingerprints of `NestedSet`s and instead always flattened the sets.

The new `fingerprint` method on `EmptyFilesSupplier` makes it possible to drop the call to `Runfiles#getEmptyFilenames`, which would still end up flattening the sets.

See https://groups.google.com/g/bazel-discuss/c/KrUg6ZPky80

Closes bazelbuild#18384.

PiperOrigin-RevId: 534724771
Change-Id: I7b39a1fa2c7c5904b186cc2d343b2b6432b05ad4
@iancha1992
Copy link
Member

iancha1992 commented Jun 22, 2023

@bazel-io fork 6.3.0

@bazel-io bazel-io mentioned this pull request Jun 22, 2023
Closed
@iancha1992 iancha1992 mentioned this pull request Jun 22, 2023
Closed
@iancha1992
Copy link
Member

iancha1992 commented Jun 23, 2023 •
edited
Loading

@lberki @fmeum
We're currently trying to cherry-pick this to release-6.3.0. However, looks like the change for "src/main/java/com/google/devtools/build/lib/exec/BUILD" is missing in the master branch (See https://github.com/bazelbuild/bazel/pull/18384/files#diff-53d9849f4a8d299e6b809d62ff433b94e34acc1292e38306bbf0cc97f029b808) I'd expect it to be there since this was merged to the master branch. Please advise on how to proceed to cherry-pick this. Thanks.

@keertk keertk mentioned this pull request Jun 28, 2023
Closed
@keertk
Copy link
Member

keertk commented Jul 10, 2023

@fmeum could you submit a PR to cherry-pick this into 6.3 please? (if needed as part of #18678)

@fmeum
Copy link
Collaborator Author

fmeum commented Jul 11, 2023

@keertk Since the other cherry-picks have been reverted/dropped, I don't think it makes sense to include this one.

@keertk keertk mentioned this pull request Jul 11, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@lberki lberki lberki approved these changes

@haxorz haxorz Awaiting requested review from haxorz

Assignees

No one assigned

Labels

team-Configurability platforms, toolchains, cquery, select(), config transitions team-Performance Issues for Performance teams team-Rules-Python Native rules for Python

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@fmeum @iancha1992 @keertk @haxorz @lberki @sgowroji