Do not retain a name string for generated module maps - #789
Conversation
|
So for monorepos, this should be a no-op, but for normal repos it would properly handle external repositories? |
fmeum
changed the title
|
@trybka Yes, that's the plan. @lilygorsheneva Please take a look, this is another attempt at #633. |
|
Looks good to me; I'll benchmark this to make sure it doesn't cause the regression again before we submit |
|
Getting a lot of analysis failures when building protobuf (and therefore everything that depends on it); not sure if it's a widespread issue or if it's proto-specific. I don't think I'll get to the bottom of this one today, i'll look at it again on Monday. |
|
It might be proto-specific, but also protos leverage module compilation so not surprising that it broke there first. Looks like an aspect might be involved, but the error is |
ec895f4 to
879bbf9
Compare
|
I pushed a direct fix for aspects. If that still causes a memory regression, I can look into optimizing that case as well. |
|
Still seeing some memory usage impact (over the threshold at which post-merge analysis will alert). Asking around for advice internally. |
|
I could try to only store the new name for an aspect-created label instead of the full label. |
|
@lilygorsheneva I refactored the build variable logic to retain the strings higher up, hope that helps. If it doesn't, bazelbuild/bazel#30682 might help in the future. |
Module maps generated for a target are named after the target's label. Since the generated module map file is declared by that very target, the name can be derived on demand from the file's owner label, which the File object already retains. This fixes two issues at once: * The name string previously stored in the module map struct (one per target) no longer needs to be retained. This achieves the memory savings of bazelbuild#633, which was rolled back because retaining Labels instead of strings increased memory usage overall - deriving the name on demand retains nothing. * Module map names were generated as 'workspace_name + "//" + package + ":" + name', which Label() fails to parse for targets in external repositories (e.g. 'rules_cc+//pkg:lib' results in 'invalid package name'), breaking header module compilation for all such targets. Module names are now the unambiguous canonical label string, except that main-repository labels drop the leading '@@' ('//pkg:lib'), preserving the traditional module name format for the main repository. Names of external-repository targets keep the '@@' and are thus valid label strings; this changes their module names, but header module compilation never worked for them due to the Label() parsing failure. The new get_module_map_label() helper returns file.owner directly for derived names and restores the '@@' prefix for explicitly named main-repository module maps (such as separate module maps). Special module maps (the crosstool module map and ObjC internal module maps) keep their explicitly provided names.
The .pcm header module artifacts are declared from the label synthesized via same_package_label(name), while the module compile action's output basename is now derived from the module map file's owner. When compile() is called with a name that differs from the declaring target's name - as protobuf's aspects do (e.g. 'foo.upb_minitable') - these two paths diverge and the pre-declared .pcm artifact is left without a generating action, failing analysis with 'The following files have no generating action: .../_objs/x.y/x.y.pic.pcm'. It would also give all module maps generated by multiple compile() calls on the same target the same owner-derived module name. Fall back to storing an explicit name, in the format produced by get_module_map_name, whenever the file's owner differs from the synthesized label. Plain rule targets still pay no memory cost as their owner matches the label and the name remains None.
get_specific_compile_build_variables runs once per compile action, so deriving the module name from the module map file's owner there created a fresh string for every action instead of the single one that was previously shared through the module map struct. For targets with more than one source file this costs more than storing the name once did. The module name, the module map file and the dependent module map files are the same for all compile actions of a target, so set them in setup_common_compile_build_variables instead. They then live once in the shared parent variables rather than in the variables retained by every action, which also shrinks each action's variables by three entries. Only the separate module compile action uses a different module name, so get_specific_compile_build_variables keeps a module_name parameter to override it. Its feature_configuration parameter is now unused and is removed.
1e9ff88 to
6884865
Compare
|
At a summit; can't guarantee I'll have time to run evals |
3 participants
Module maps generated for a target are named after the target's label. Since the generated module map file is declared by that very target, the name can be derived on demand from the file's owner label, which the
Fileobject already retains. Thenamefield of the module map struct becomes optional, withNonemeaning "named after the label of the target that generated the module map file", and two central helpers (get_module_map_name,get_module_map_label) derive the string form and theLabelform where needed.This fixes the round-trip bug originally addressed in #633, but avoids the memory regression that caused it to be rolled back in b81b6e4 (because retaining
Labelobjects increased memory usage overall). Deriving the name on demand retains nothing extra as the owner label is already retained by theFileand all name materializations are transient only (such as inmap_eachcallbacks).Names are now the unambiguous canonical label string, except that main repository labels drop the leading
@@(//pkg:lib), keeping the traditional module name format. Special module maps (the crosstool module map, ObjC internal module maps) keep their explicitly provided names.