Fix analysis time regression with Bzlmod lockfile

[fmeum]

This commit fixes a regression in analysis time caused by 19c0c80. Since BazelModuleResolutionEvent and ModuleExtensionResolutionEvent are both marked as storeForReplay, they are stored in a nested set for essentially all analysis phase nodes. This resulted in frequent (i.e., per target) calls to their hashCode methods, which are not cached and delegated to the likewise uncached methods on large ImmutableMap and ImmutableTable instances.

Since there is no need for these events to be deduplicated, switching to reference equality resolves this issue.

The following analysis phase measurements for a synthetic project with a single "hub and spokes" module extension and 2,000 repos illustrate the effect:

• without lockfile: 4.3s
• with lockfile before this commit: 8.3s
• with lockfile after this commit: 4.2s

Fixes #19952

[meteorcloudy]

Thank you!!

[meteorcloudy]

@bazel-io fork 7.0.0

[Wyverald]

Could we achieve the same result by adding @Memoized to hashCode()? That way we won't lose the benefit of using AutoValue.

Also, that would give us a convenient place to document why this is needed at all. This is important because I could certainly imagine myself later looking at this class, thinking "huh, this is a prime candidate to turn into an AutoValue", and reintroducing the performance regression.

[fmeum]

Could we achieve the same result by adding @Memoized to hashCode()? That way we won't lose the benefit of using AutoValue.

That's what I tried first, but ultimately decided against:

• All the other event classes I could find in Bazel also use reference equality, so using value-based equality here would actually decrease consistency.
• The Bzlmod events are fired in a way that already prevents duplicates, so the computation and storage of hash codes would still be wasted. equals could still be called and may turn out to be expensive depending on field order.
• Without hashCode and equals, AutoValue really only gets rid of the field declarations, which are much less error-prone to maintain manually.

[fmeum]

Just saw your edit, I will add comments and think about ways to add test coverage (seems difficult though).

I could see adding comments to the classes be as effective as adding them to the @Memoized annotations as the class as where one would place @AutoValue. What do you think?

[Wyverald]

I'm okay with skipping AutoValue altogether if we document it at the class level. I wouldn't worry about regression testing in this case.

Alternatively, we could also just force-override hashCode and equals to be their trivial version (using super.hashCode() and super.equals(other)).

I have a slight preference for keeping the AutoValue idiom (using X.create() etc), but it's not strong opinion at all.

[fmeum]

I added class comments as well as the static create methods. Is that what you meant with your last paragraph?