Skip to content

Intern empty Depsets #19387

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 1 commit into from
Closed

Intern empty Depsets #19387

fmeum wants to merge 1 commit into from

Conversation

@fmeum
Copy link
Collaborator

@fmeum fmeum commented Aug 31, 2023 •
edited
Loading

Empty Depsets are interned per order, which decreases allocations as well as retained heap when a Depset is stored in a struct in a provider (providers with schema already unwrap most Depsets).

Fixes #19380

@github-actions github-actions bot added the awaiting-review PR is awaiting review from an assigned reviewer label Aug 31, 2023
@fmeum fmeum marked this pull request as draft August 31, 2023 22:18
@fmeum fmeum force-pushed the intern-empty-depsets branch 2 times, most recently from 770ef2b to b31b1d9 Compare September 1, 2023 08:15
@fmeum fmeum marked this pull request as ready for review September 1, 2023 08:43
@fmeum fmeum requested a review from comius September 1, 2023 08:43
@iancha1992 iancha1992 added the team-Rules-API API for writing rules/aspects: providers, runfiles, actions, artifacts label Sep 1, 2023
comius
comius requested changes Sep 6, 2023
View reviewed changes
src/main/java/com/google/devtools/build/lib/collect/nestedset/Depset.java Outdated
Comment on lines 104 to 108
private static final ImmutableMap<Order, Depset> EMPTY_DEPSETS =
Arrays.stream(Order.values())
.map(Order::emptySet)
.collect(toImmutableEnumMap(NestedSet::getOrder, e -> new Depset(null, e)));

Copy link
Contributor

@comius comius Sep 6, 2023

Choose a reason for hiding this comment

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

Is it possible to follow the same pattern as for NestedSets? That is to add emptyDepset next to emptySet to Order class.

That would satisfy the requirement to have an empty depset interned for each Order element.

It also seems not to create a problem with a cycle, because Depset, Nestedset and Order are in the same java_library.

Implementing it in Order, would make the code a bit simpler (not introducing 2 different implementation for NestedSet and Depset) and making it easier to follow.

Copy link
Collaborator Author

@fmeum fmeum Sep 6, 2023

Choose a reason for hiding this comment

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

Done. This required some further changes to break cyclic dependencies between Depset's and Order's static initializers.

Let me know if I should also add @SerializationConstants for the empty depsets - I don't know whether they could be subject to serialization.

@comius comius added awaiting-user-response Awaiting a response from the author and removed awaiting-review PR is awaiting review from an assigned reviewer labels Sep 6, 2023
@fmeum fmeum force-pushed the intern-empty-depsets branch from b31b1d9 to 9ecb092 Compare September 6, 2023 10:44
@fmeum fmeum requested a review from comius September 6, 2023 10:44
@brentleyjones brentleyjones added awaiting-review PR is awaiting review from an assigned reviewer and removed awaiting-user-response Awaiting a response from the author labels Sep 6, 2023
@comius comius 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 Sep 6, 2023
@fmeum
Copy link
Collaborator Author

fmeum commented Sep 6, 2023

@bazel-io flag

@bazel-io bazel-io added the potential release blocker Flagged by community members using "@bazel-io flag". Should be added to a release blocker milestone label Sep 6, 2023
@copybara-service copybara-service bot closed this in fa0ff49 Sep 6, 2023
@github-actions github-actions bot removed the awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally label Sep 6, 2023
@fmeum fmeum deleted the intern-empty-depsets branch September 6, 2023 18:03
@iancha1992
Copy link
Member

iancha1992 commented Sep 6, 2023

@bazel-io fork 6.4.0

@bazel-io bazel-io mentioned this pull request Sep 6, 2023
Closed
@bazel-io bazel-io removed the potential release blocker Flagged by community members using "@bazel-io flag". Should be added to a release blocker milestone label Sep 6, 2023
@iancha1992
Copy link
Member

iancha1992 commented Sep 6, 2023

@fmeum @comius
We tried to cherry-pick to release-6.4.0. But there are some merge conflicts with src/main/java/com/google/devtools/build/lib/collect/nestedset/Depset.java

  1. We currently have in release-6.4.0 below
private Depset(ElementType elemType, NestedSet<?> set) {
    this.elemType = Preconditions.checkNotNull(elemType, "element type cannot be null");
    this.set = set;
  }

But, we expected before the cherry-pick to be:

private Depset(@Nullable Class<?> elemClass, NestedSet<?> set) {
    this.elemClass = elemClass;
    this.set = set;
}
  1. We expected before the cherry-pick:
public static <T> Depset of(Class<T> elemClass, NestedSet<T> set) {
    Preconditions.checkNotNull(elemClass, "elemClass cannot be null");
    // Having a shared EMPTY_DEPSET instance, could reduce working heap. Empty depsets are not
    // retained though, because of optimization in StarlarkProvider.optimizeField.
    return new Depset(set.isEmpty() ? null : ElementType.getTypeClass(elemClass), set);
}

but we currently have in the release-6.4.0 branch, below:

public static <T> Depset of(ElementType elemType, NestedSet<T> set) {
    return new Depset(elemType, set);
}

cc: @bazelbuild/triage

fmeum added a commit to fmeum/bazel that referenced this pull request Sep 7, 2023
@fmeum
Intern empty Depsets
a532278 
Empty `Depset`s are interned per order, which decreases allocations as well as retained heap when a `Depset` is stored in a `struct` in a provider (providers with schema already unwrap most `Depset`s).

Fixes bazelbuild#19380

Closes bazelbuild#19387.

PiperOrigin-RevId: 563104923
Change-Id: If66fb4a108ef7569a4f95e7af3a74ac84d1c4636
@fmeum fmeum mentioned this pull request Sep 7, 2023
Merged
@fmeum
Copy link
Collaborator Author

fmeum commented Sep 7, 2023

@iancha1992 I submitted a resolution of these conflicts as #19443.

iancha1992 pushed a commit that referenced this pull request Sep 11, 2023
@fmeum
[6.4.0] Intern empty Depsets (#19443)
bc8bb95 
Empty `Depset`s are interned per order, which decreases allocations as
well as retained heap when a `Depset` is stored in a `struct` in a
provider (providers with schema already unwrap most `Depset`s).

Fixes #19380

Closes #19387.

PiperOrigin-RevId: 563104923
Change-Id: If66fb4a108ef7569a4f95e7af3a74ac84d1c4636
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@comius comius comius approved these changes

Assignees

No one assigned

Labels

team-Rules-API API for writing rules/aspects: providers, runfiles, actions, artifacts

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Empty depsets should be optimized out of structs the same as they are optimized out of providers

5 participants

@fmeum @iancha1992 @comius @brentleyjones @bazel-io