From 8358276f718593a299a7a320adc713e3804c166e Mon Sep 17 00:00:00 2001 From: Fabian Meumertzheim Date: Wed, 1 Nov 2023 21:36:25 -0700 Subject: [PATCH] Fix analysis time regression with Bzlmod lockfile This commit fixes a regression in analysis time caused by 19c0c809abbe4bc70d3d6b493ff966dd41c54768. 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 Closes #19970. PiperOrigin-RevId: 578734010 Change-Id: I870867c5c509389632793b0d5fe5c43ddc6176f3 --- .../bzlmod/BazelModuleResolutionEvent.java | 32 +++++++++++---- .../ModuleExtensionResolutionEvent.java | 39 +++++++++++++++---- .../devtools/build/lib/events/Reportable.java | 3 ++ 3 files changed, 59 insertions(+), 15 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BazelModuleResolutionEvent.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BazelModuleResolutionEvent.java index b2e0f6f3a5b40f..cd9c1e6d5bcddf 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BazelModuleResolutionEvent.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BazelModuleResolutionEvent.java @@ -14,7 +14,6 @@ package com.google.devtools.build.lib.bazel.bzlmod; -import com.google.auto.value.AutoValue; import com.google.common.collect.ImmutableTable; import com.google.devtools.build.lib.events.ExtendedEventHandler.Postable; @@ -22,20 +21,39 @@ * After resolving bazel module this event is sent from {@link BazelDepGraphFunction} holding the * lockfile value with module updates, and the module extension usages. It will be received in * {@link BazelLockFileModule} to be used to update the lockfile content + * + *

Instances of this class are retained in Skyframe nodes and subject to frequent {@link + * java.util.Set}-based deduplication. As such, it must have a cheap implementation of {@link + * #hashCode()} and {@link #equals(Object)}. It currently uses reference equality since the logic + * that creates instances of this class already ensures that there is only one instance per build. */ -@AutoValue -public abstract class BazelModuleResolutionEvent implements Postable { +public final class BazelModuleResolutionEvent implements Postable { + + private final BazelLockFileValue lockfileValue; + private final ImmutableTable + extensionUsagesById; + + private BazelModuleResolutionEvent( + BazelLockFileValue lockfileValue, + ImmutableTable extensionUsagesById) { + this.lockfileValue = lockfileValue; + this.extensionUsagesById = extensionUsagesById; + } public static BazelModuleResolutionEvent create( BazelLockFileValue lockFileValue, ImmutableTable extensionUsagesById) { - return new AutoValue_BazelModuleResolutionEvent(lockFileValue, extensionUsagesById); + return new BazelModuleResolutionEvent(lockFileValue, extensionUsagesById); } - public abstract BazelLockFileValue getLockfileValue(); + public BazelLockFileValue getLockfileValue() { + return lockfileValue; + } - public abstract ImmutableTable - getExtensionUsagesById(); + public ImmutableTable + getExtensionUsagesById() { + return extensionUsagesById; + } @Override public boolean storeForReplay() { diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionResolutionEvent.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionResolutionEvent.java index 6b93cc4e77332a..893608bef70afe 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionResolutionEvent.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionResolutionEvent.java @@ -14,30 +14,53 @@ package com.google.devtools.build.lib.bazel.bzlmod; -import com.google.auto.value.AutoValue; import com.google.devtools.build.lib.events.ExtendedEventHandler.Postable; /** * After evaluating any module extension this event is sent from {@link SingleExtensionEvalFunction} * holding the extension id and the resolution data LockFileModuleExtension. It will be received in - * {@link BazelLockFileModule} to be used to update the lockfile content + * {@link BazelLockFileModule} to be used to update the lockfile content. + * + *

Instances of this class are retained in Skyframe nodes and subject to frequent {@link + * java.util.Set}-based deduplication. As such, it must have a cheap implementation of {@link + * #hashCode()} and {@link #equals(Object)}. It currently uses reference equality since the logic + * that creates instances of this class already ensures that there is only one instance per + * extension id. */ -@AutoValue -public abstract class ModuleExtensionResolutionEvent implements Postable { +public final class ModuleExtensionResolutionEvent implements Postable { + + private final ModuleExtensionId extensionId; + private final ModuleExtensionEvalFactors extensionFactors; + private final LockFileModuleExtension moduleExtension; + + private ModuleExtensionResolutionEvent( + ModuleExtensionId extensionId, + ModuleExtensionEvalFactors extensionFactors, + LockFileModuleExtension moduleExtension) { + this.extensionId = extensionId; + this.extensionFactors = extensionFactors; + this.moduleExtension = moduleExtension; + } public static ModuleExtensionResolutionEvent create( ModuleExtensionId extensionId, ModuleExtensionEvalFactors extensionFactors, LockFileModuleExtension lockfileModuleExtension) { - return new AutoValue_ModuleExtensionResolutionEvent( + return new ModuleExtensionResolutionEvent( extensionId, extensionFactors, lockfileModuleExtension); } - public abstract ModuleExtensionId getExtensionId(); + public ModuleExtensionId getExtensionId() { + return extensionId; + } - public abstract ModuleExtensionEvalFactors getExtensionFactors(); + public ModuleExtensionEvalFactors getExtensionFactors() { + return extensionFactors; + } - public abstract LockFileModuleExtension getModuleExtension(); + public LockFileModuleExtension getModuleExtension() { + return moduleExtension; + } @Override public boolean storeForReplay() { diff --git a/src/main/java/com/google/devtools/build/lib/events/Reportable.java b/src/main/java/com/google/devtools/build/lib/events/Reportable.java index 4601fc4ef172c1..cf8f757c5a665c 100644 --- a/src/main/java/com/google/devtools/build/lib/events/Reportable.java +++ b/src/main/java/com/google/devtools/build/lib/events/Reportable.java @@ -47,6 +47,9 @@ public interface Reportable { * *

This method is not relevant for events which do not originate from {@link * com.google.devtools.build.skyframe.SkyFunction} evaluation. + * + *

Classes returning {@code true} should have cheap {@link Object#hashCode()} and {@link + * Object#equals(Object)} implementations. */ default boolean storeForReplay() { return false;