8000 Move package and package piece overhead/computation steps/description… · coderabbit-test/bazel@da2c551 · GitHub
[go: up one dir, main page]
Skip to content

Commit da2c551

Browse files
tetrominocopybara-github
tetromino
authored and
copybara-github
committed
Move package and package piece overhead/computation steps/description getters/setters to base classes
In preparation for refactoring of PackageFactory and related machinery. Move package / package piece-specific initialization from overrides of finishBuild to overrides of a new method, packageoidInitializationHook, so that finishBuild can run overhead estimation on a fully initialized packageoid. (This also has the nice side effect of making the order of operations in finishBuild much easier to follow.) Working towards bazelbuild#23852 PiperOrigin-RevId: 746193550 Change-Id: I2c3ff42660642b6f4fb958dfac12381f0fbfc743
1 parent 492f3da commit da2c551

File tree

6 files changed

+116
-62
lines changed

6 files changed

+116
-62
lines changed

‎src/main/java/com/google/devtools/build/lib/packages/Package.java

Lines changed: 20 additions & 46 deletions
Original file line numberDiff line numberDiff line change
@@ -72,7 +72,6 @@
7272
import java.util.Map;
7373
import java.util.Optional;
7474
import java.util.OptionalInt;
75-
import java.util.OptionalLong;
7675
import java.util.SequencedMap;
7776
import java.util.Set;
7877
import java.util.concurrent.Semaphore;
@@ -141,21 +140,12 @@ public enum ConfigSettingVisibilityPolicy {
141140
*/
142141
private static final String DUMMY_WORKSPACE_NAME_FOR_BZLMOD_PACKAGES = "__dummy_workspace_bzlmod";
143142

144-
/** Sentinel value for package overhead being empty. */
145-
private static final long PACKAGE_OVERHEAD_UNSET = -1;
146-
147143
// ==== General package metadata fields ====
148144

149145
private final Metadata metadata;
150146

151147
private final Declarations declarations;
152148

153-
/**
154-
* A rough approximation of the memory and general accounting costs associated with a loaded
155-
* package. A value of -1 means it is unset. Stored as a long to take up less memory per pkg.
156-
*/
157-
private long packageOverhead = PACKAGE_OVERHEAD_UNSET;
158-
159149
// ==== Fields specific to external package / WORKSPACE logic ====
160150

161151
/**
@@ -344,13 +334,6 @@ private static ImmutableList<Label> computeTransitiveLoads(Iterable<Module> dire
344334
return ImmutableList.copyOf(loads);
345335
}
346336

347-
/** Returns package overhead as configured by the configured {@link PackageOverheadEstimator}. */
348-
public OptionalLong getPackageOverhead() {
349-
return packageOverhead == PACKAGE_OVERHEAD_UNSET
350-
? OptionalLong.empty()
351-
: OptionalLong.of(packageOverhead);
352-
}
353-
354337
// ==== Accessors specific to external package / WORKSPACE logic ====
355338

356339
/**
@@ -412,11 +395,6 @@ OptionalInt getFirstWorkspaceSuffixRegisteredToolchain() {
412395

413396
// ==== Target and macro accessors ====
414397

415-
/** Returns an (immutable, ordered) view of all the targets belonging to this package. */
416-
public ImmutableSortedMap<String, Target> getTargets() {
417-
return targets;
418-
}
419-
420398
/**
421399
* Returns a (read-only, ordered) iterable of all the targets belonging to this package which are
422400
* instances of the specified class.
@@ -612,6 +590,11 @@ public String toString() {
612590
+ (targets != null ? getTargets(Rule.class) : "initializing...");
613591
}
614592

593+
@Override
594+
public String getShortDescription() {
595+
return "package " + getPackageIdentifier().getCanonicalForm();
596+
}
597+
615598
/**
616599
* Dumps the package for debugging. Do not depend on the exact format/contents of this debugging
617600
* output.
@@ -957,6 +940,10 @@ void setPackageFunctionUsed() {
957940
packageFunctionUsed = true;
958941
}
959942

943+
public Set<Target> getTargets() {
944+
return recorder.getTargets();
945+
}
946+
960947
/** Adds an environment group to the package. Not valid within symbolic macros. */
961948
void addEnvironmentGroup(
962949
String name,
@@ -1157,9 +1144,7 @@ protected void finalBuilderValidationHook() {
11571144
}
11581145

11591146
@Override
1160-
public Packageoid finishBuild() {
1161-
var unused = super.finishBuild();
1162-
1147+
protected void packageoidInitializationHook() {
11631148
// Finish Package.Declarations construction.
11641149
if (pkg.getDeclarations().directLoads == null
11651150
&& pkg.getDeclarations().transitiveLoads == null) {
@@ -1168,8 +1153,6 @@ public Packageoid finishBuild() {
11681153
}
11691154
pkg.getDeclarations().workspaceName = workspaceName;
11701155
pkg.getDeclarations().makeEnv = ImmutableMap.copyOf(makeEnv);
1171-
1172-
return pkg;
11731156
}
11741157

11751158
AbstractBuilder(
@@ -1182,6 +1165,7 @@ public Packageoid finishBuild() {
11821165
String workspaceName,
11831166
RepositoryMapping mainRepositoryMapping,
11841167
@Nullable Semaphore cpuBoundSemaphore,
1168+
PackageOverheadEstimator packageOverheadEstimator,
11851169
@Nullable ImmutableMap<Location, String> generatorMap,
11861170
@Nullable Globber globber,
11871171
boolean enableNameConflictChecking,
@@ -1194,6 +1178,7 @@ public Packageoid finishBuild() {
11941178
workspaceName,
11951179
mainRepositoryMapping,
11961180
cpuBoundSemaphore,
1181+
packageOverheadEstimator,
11971182
generatorMap,
11981183
globber,
11991184
enableNameConflictChecking,
@@ -1261,9 +1246,6 @@ default boolean precomputeTransitiveLoads() {
12611246
private final HashMap<RepositoryName, LinkedHashMap<String, RepositoryName>>
12621247
externalPackageRepositoryMappings = new HashMap<>();
12631248

1264-
/** Estimates the package overhead of this package. */
1265-
private final PackageOverheadEstimator packageOverheadEstimator;
1266-
12671249
// The snapshot of {@link #targets} for use in rule finalizer macros. Contains all
12681250
// non-finalizer-instantiated rule targets (i.e. all rule targets except for those instantiated
12691251
// in a finalizer or in a macro called from a finalizer).
@@ -1320,11 +1302,11 @@ private Builder(
13201302
workspaceName,
13211303
mainRepositoryMapping,
13221304
cpuBoundSemaphore,
1305+
packageOverheadEstimator,
13231306
generatorMap,
13241307
globber,
13251308
enableNameConflictChecking,
13261309
trackFullMacroInformation);
1327-
this.packageOverheadEstimator = packageOverheadEstimator;
13281310
}
13291311

13301312
/** Retrieves this object from a Starlark thread. Returns null if not present. */
@@ -1435,10 +1417,6 @@ void setComputationSteps(long n) {
14351417
getPackage().computationSteps = n;
14361418
}
14371419

1438-
public Set<Target> getTargets() {
1439-
return recorder.getTargets();
1440-
}
1441-
14421420
void replaceTarget(Target newTarget) {
14431421
Preconditions.checkArgument(
14441422
newTarget.getPackage() == pkg, // pointer comparison since we're constructing `pkg`
@@ -1607,7 +1585,13 @@ public Builder buildPartial() throws NoSuchPackageException {
16071585

16081586
@Override
16091587
public Package finishBuild() {
1610-
Package pkg = (Package) super.finishBuild();
1588+
return (Package) super.finishBuild();
1589+
}
1590+
1591+
@Override
1592+
protected void packageoidInitializationHook() {
1593+
super.packageoidInitializationHook();
1594+
Package pkg = getPackage();
16111595
pkg.macroNamespaceViolatingTargets =
16121596
ImmutableMap.copyOf(recorder.getMacroNamespaceViolatingTargets());
16131597
pkg.targetsToDeclaringMacro =
@@ -1634,16 +1618,6 @@ public Package finishBuild() {
16341618
externalPackageRepositoryMappings.forEach(
16351619
(k, v) -> repositoryMappingsBuilder.put(k, ImmutableMap.copyOf(v)));
16361620
pkg.externalPackageRepositoryMappings = repositoryMappingsBuilder.buildOrThrow();
1637-
OptionalLong overheadEstimate = packageOverheadEstimator.estimatePackageOverhead(pkg);
1638-
pkg.packageOverhead = overheadEstimate.orElse(PACKAGE_OVERHEAD_UNSET);
1639-
1640-
// Verify that we haven't introduced new errors on the builder since the call to
1641-
// finalBuilderValidationHook().
1642-
if (containsErrors()) {
1643-
checkState(pkg.containsErrors(), "Package.Builder error status not propagated to Package");
1644-
}
1645-
1646-
return pkg;
16471621
}
16481622

16491623
/** Completes package construction. Idempotent. */

‎src/main/java/com/google/devtools/build/lib/packages/PackageOverheadEstimator.java

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -17,16 +17,16 @@
1717

1818
/**
1919
* Estimates "package overhead", which is a rough approximation of the memory and general accounting
20-
* costs associated with a loaded package.
20+
* costs associated with a loaded package or package piece.
2121
*
2222
* <p>Estimates are not intended to be perfect but should be reproducible. Some things may be
2323
* over-accounted, some things under, with the expectation that it all comes out roughly even in the
2424
* end.
2525
*/
2626
public interface PackageOverheadEstimator {
2727

28-
PackageOverheadEstimator NOOP_ESTIMATOR = pkg -> OptionalLong.empty();
28+
PackageOverheadEstimator NOOP_ESTIMATOR = (pkg) -> OptionalLong.empty();
2929

3030
/** Returns the estimated package overhead, or empty if not calculated. */
31-
OptionalLong estimatePackageOverhead(Package pkg);
31+
OptionalLong estimatePackageOverhead(Packageoid pkg);
3232
}

‎src/main/java/com/google/devtools/build/lib/packages/PackagePiece.java

Lines changed: 19 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,6 @@
2020
import com.google.common.annotations.VisibleForTesting;
2121
import com.google.common.collect.ImmutableMap;
2222
import com.google.common.collect.ImmutableSet;
23-
import com.google.common.collect.ImmutableSortedMap;
2423
import com.google.common.collect.Iterables;
2524
import com.google.devtools.build.lib.cmdline.Label;
2625
import com.google.devtools.build.lib.cmdline.LabelSyntaxException;
@@ -68,14 +67,6 @@ public Identifier getIdentifier() {
6867
*/
6968
public abstract PackagePiece.ForBuildFile getPackagePieceForBuildFile();
7069

71-
/**
72-
* Returns an (immutable, ordered) view of all the targets belonging to this package piece.
73-
* Doesn't search in any other package pieces.
74-
*/
75-
public ImmutableSortedMap<String, Target> getTargets() {
76-
return targets;
77-
}
78-
7970
/**
8071
* Returns a (read-only, ordered) iterable of all the targets belonging to this package piece
8172
* which are instances of the specified class. Doesn't search in any other package pieces.
@@ -154,6 +145,11 @@ public String toString() {
154145
+ (targets != null ? getTargets(Rule.class) : "initializing...");
155146
}
156147

148+
@Override
149+
public String getShortDescription() {
150+
return "package piece " + getIdentifier();
151+
}
152+
157153
private PackagePiece(Identifier identifier) {
158154
this.identifier = identifier;
159155
}
@@ -290,6 +286,7 @@ public static Builder newBuilder(
290286
RepositoryMapping repositoryMapping,
291287
RepositoryMapping mainRepositoryMapping,
292288
@Nullable Semaphore cpuBoundSemaphore,
289+
PackageOverheadEstimator packageOverheadEstimator,
293290
@Nullable ImmutableMap<Location, String> generatorMap,
294291
@Nullable ConfigSettingVisibilityPolicy configSettingVisibilityPolicy,
295292
@Nullable Globber globber,
@@ -315,6 +312,7 @@ public static Builder newBuilder(
315312
workspaceName,
316313
mainRepositoryMapping,
317314
cpuBoundSemaphore,
315+
packageOverheadEstimator,
318316
generatorMap,
319317
globber,
320318
enableNameConflictChecking,
@@ -370,6 +368,7 @@ private Builder(
370368
String workspaceName,
371369
RepositoryMapping mainRepositoryMapping,
372370
@Nullable Semaphore cpuBoundSemaphore,
371+
PackageOverheadEstimator packageOverheadEstimator,
373372
@Nullable ImmutableMap<Location, String> generatorMap,
374373
@Nullable Globber globber,
375374
boolean enableNameConflictChecking,
@@ -384,6 +383,7 @@ private Builder(
384383
workspaceName,
385384
mainRepositoryMapping,
386385
cpuBoundSemaphore,
386+
packageOverheadEstimator,
387387
generatorMap,
388388
globber,
389389
enableNameConflictChecking,
@@ -464,6 +464,7 @@ public static Builder newBuilder(
464464
RepositoryMapping repositoryMapping,
465465
RepositoryMapping mainRepositoryMapping,
466466
@Nullable Semaphore cpuBoundSemaphore,
467+
PackageOverheadEstimator packageOverheadEstimator,
467468
@Nullable ImmutableMap<Location, String> generatorMap,
468469
boolean enableNameConflictChecking,
469470
boolean trackFullMacroInformation) {
@@ -473,6 +474,7 @@ public static Builder newBuilder(
473474
simplifyUnconditionalSelectsInRuleAttrs,
474475
mainRepositoryMapping,
475476
cpuBoundSemaphore,
477+
packageOverheadEstimator,
476478
generatorMap,
477479
enableNameConflictChecking,
478480
trackFullMacroInformation);
@@ -542,17 +544,21 @@ public Builder buildPartial() throws NoSuchPackageException {
542544

543545
@Override
544546
public ForMacro finishBuild() {
545-
ForMacro forMacro = (ForMacro) super.finishBuild();
546-
forMacro.macroNamespaceViolations =
547+
return (ForMacro) super.finishBuild();
548+
}
549+
550+
@Override
551+
protected void packageoidInitializationHook() {
552+
getPackagePiece().macroNamespaceViolations =
547553
ImmutableSet.copyOf(recorder.getMacroNamespaceViolatingTargets().keySet());
548-
return forMacro;
549554
}
550555

551556
private Builder(
552557
ForMacro forMacro,
553558
boolean simplifyUnconditionalSelectsInRuleAttrs,
554559
RepositoryMapping mainRepositoryMapping,
555560
@Nullable Semaphore cpuBoundSemaphore,
561+
PackageOverheadEstimator packageOverheadEstimator,
556562
@Nullable ImmutableMap<Location, String> generatorMap,
557563
boolean enableNameConflictChecking,
558564
boolean trackFullMacroInformation) {
@@ -564,6 +570,7 @@ private Builder(
564570
forMacro.getPackagePieceForBuildFile().getDeclarations().getWorkspaceName(),
565571
mainRepositoryMapping,
566572
cpuBoundSemaphore,
573+
packageOverheadEstimator,
567574
generatorMap,
568575
/* globber= */ null,
569576
enableNameConflictChecking,

‎src/main/java/com/google/devtools/build/lib/packages/Packageoid.java

Lines changed: 32 additions & 0 deletions
< 37BF tr class="diff-line-row">
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@
2020
import com.google.devtools.build.lib.cmdline.PackageIdentifier;
2121
import com.google.devtools.build.lib.packages.TargetRecorder.MacroNamespaceViolationException;
2222
import com.google.devtools.build.lib.server.FailureDetails.FailureDetail;
23+
import java.util.OptionalLong;
2324
import javax.annotation.Nullable;
2425

2526
/**
@@ -36,6 +37,9 @@
3637
* machinery.
3738
*/
3839
public abstract class Packageoid {
40+
/** Sentinel value for package overhead being empty. */
41+
protected static final long PACKAGE_OVERHEAD_UNSET = -1;
42+
3943
// ==== Common metadata fields ====
4044

4145
/**
@@ -55,6 +59,12 @@ public abstract class Packageoid {
5559

5660
protected long computationSteps = 0;
5761

62+
/**
63+
* A rough approximation of the memory and general accounting costs associated with a loaded
64+
* packageoid. A value of -1 means it is unset. Stored as a long to take up less memory per pkg.
65+
*/
66+
protected long packageOverhead = PACKAGE_OVERHEAD_UNSET;
67+
5868
// ==== Common target and macro fields ====
5969

6070
/**
@@ -96,6 +106,21 @@ public PackageIdentifier getPackageIdentifier() {
96106
*/
97107
public abstract Package.Declarations getDeclarations();
98108

109+
/**
110+
* Returns a short, lower-case description of this packageoid, e.g. for use in logging and error
111+
* messages.
112+
*/
113+
public abstract String getShortDescription();
114+
115+
/**
116+
* Returns an (immutable, ordered) view of all the targets belonging to this packageoid. Note that
117+
* if this packageoid is a package piece, this method does not search for targets in any other
118+
* package pieces.
119+
*/
120+
public ImmutableSortedMap<String, Target> getTargets() {
121+
return targets;
122+
}
123+
99124
/**
100125
* Returns true if errors were encountered during evaluation of this packageoid.
101126
*
@@ -140,6 +165,13 @@ public long getComputationSteps() {
140165
return computationSteps;
141166
}
142167

168+
/** Returns package overhead as configured by the configured {@link PackageOverheadEstimator}. */
169+
public OptionalLong getPackageOverhead() {
170+
return packageOverhead == PACKAGE_OVERHEAD_UNSET
171+
? OptionalLong.empty()
172+
: OptionalLong.of(packageOverhead);
173+
}
174+
143175
/**
144176
* Throws {@link MacroNamespaceViolationException} if the given target (which must be a member of
145177
* this packageoid) violates macro naming rules.

0 commit comments

Comments
 (0)
0