Skip to content

Commit

Permalink
Fix analysis time regression with Bzlmod lockfile
Browse files Browse the repository at this point in the history
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 bazelbuild#19952

Closes bazelbuild#19970.

PiperOrigin-RevId: 578734010
Change-Id: I870867c5c509389632793b0d5fe5c43ddc6176f3
  • Loading branch information
fmeum authored and copybara-github committed Nov 2, 2023
1 parent d52edce commit 435d1c2
Show file tree
Hide file tree
Showing 3 changed files with 59 additions and 15 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -14,28 +14,46 @@

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;

/**
* 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
*
* <p>Instances of this class are retained in Skyframe nodes and subject to frequent {@link
* java.util.Set}-based deduplication. As such, it <b>must</b> 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<ModuleExtensionId, ModuleKey, ModuleExtensionUsage>
extensionUsagesById;

private BazelModuleResolutionEvent(
BazelLockFileValue lockfileValue,
ImmutableTable<ModuleExtensionId, ModuleKey, ModuleExtensionUsage> extensionUsagesById) {
this.lockfileValue = lockfileValue;
this.extensionUsagesById = extensionUsagesById;
}

public static BazelModuleResolutionEvent create(
BazelLockFileValue lockFileValue,
ImmutableTable<ModuleExtensionId, ModuleKey, ModuleExtensionUsage> extensionUsagesById) {
return new AutoValue_BazelModuleResolutionEvent(lockFileValue, extensionUsagesById);
return new BazelModuleResolutionEvent(lockFileValue, extensionUsagesById);
}

public abstract BazelLockFileValue getLockfileValue();
public BazelLockFileValue getLockfileValue() {
return lockfileValue;
}

public abstract ImmutableTable<ModuleExtensionId, ModuleKey, ModuleExtensionUsage>
getExtensionUsagesById();
public ImmutableTable<ModuleExtensionId, ModuleKey, ModuleExtensionUsage>
getExtensionUsagesById() {
return extensionUsagesById;
}

@Override
public boolean storeForReplay() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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.
*
* <p>Instances of this class are retained in Skyframe nodes and subject to frequent {@link
* java.util.Set}-based deduplication. As such, it <b>must</b> 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() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,9 @@ public interface Reportable {
*
* <p>This method is not relevant for events which do not originate from {@link
* com.google.devtools.build.skyframe.SkyFunction} evaluation.
*
* <p>Classes returning {@code true} should have cheap {@link Object#hashCode()} and {@link
* Object#equals(Object)} implementations.
*/
default boolean storeForReplay() {
return false;
Expand Down

0 comments on commit 435d1c2

Please sign in to comment.