-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Intern empty Depset
s
#19387
Intern empty Depset
s
#19387
Conversation
770ef2b
to
b31b1d9
Compare
private static final ImmutableMap<Order, Depset> EMPTY_DEPSETS = | ||
Arrays.stream(Order.values()) | ||
.map(Order::emptySet) | ||
.collect(toImmutableEnumMap(NestedSet::getOrder, e -> new Depset(null, e))); | ||
|
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 @SerializationConstant
s for the empty depsets - I don't know whether they could be subject to serialization.
b31b1d9
to
9ecb092
Compare
@bazel-io flag |
@bazel-io fork 6.4.0 |
@fmeum @comius
But, we expected before the cherry-pick to be:
but we currently have in the release-6.4.0 branch, below:
cc: @bazelbuild/triage |
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
@iancha1992 I submitted a resolution of these conflicts as #19443. |
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
Empty
Depset
s are interned per order, which decreases allocations as well as retained heap when aDepset
is stored in astruct
in a provider (providers with schema already unwrap mostDepset
s).Fixes #19380