Retain module map names as Labels#633
Closed
fmeum wants to merge 2 commits into
Closed
Conversation
fmeum
requested review from
armandomontanez,
c-mita,
hvadehra,
pzembrod and
trybka
as code owners
March 12, 2026 22:25
armandomontanez
added
type: bug
lilygorsheneva
approved these changes
Apr 20, 2026
hvadehra
removed their request for review
April 21, 2026 04:42
lilygorsheneva
requested changes
Apr 21, 2026
|
|
||
| if feature_configuration.is_enabled("module_maps") and cpp_module_map: | ||
| result[_VARS.MODULE_NAME] = cpp_module_map.name | ||
| result[_VARS.MODULE_NAME] = str(cpp_module_map.identifier) |
Collaborator
There was a problem hiding this comment.
I think you need to do the _stringify_module_map conversion here too.
A lot of internal Bazel integration tests failed with layering check issues when I tried this change and adding that conversion seemed to fix it.
That being said, I can't verify whether that will end up working correctly outside of Google.
With the exception of special module map names, all of them are effectively labels. This commit ensures that they are retained as `Label`s, not label string and thus: * memory usage is reduced for module maps from external repos, whose module map name previously resulted in a newly retained string; * module map names now use properly formatted external repo labels as names, which makes it more obvious what they are; * the roundtrip in header module compilation flows is avoided. This actually fixes a bug where the missing `@@` prefix for label strings resulted in incorrect or even failing `Label` calls due to main repo labels being resolved relative to the rules_cc repo and external repo labels failing the validation of the `Label` constructor.
lilygorsheneva
approved these changes
May 4, 2026
pzembrod
approved these changes
May 8, 2026
copybara-service Bot
pushed a commit
to bazelbuild/bazel
that referenced
this pull request
May 13, 2026
Copybara Import from bazelbuild/rules_cc#633 BEGIN_PUBLIC Retain module map names as `Label`s (#633) With the exception of special module map names, all of them are effectively labels. This commit ensures that they are retained as `Label`s, not label string and thus: * memory usage is reduced for module maps from external repos, whose module map name previously resulted in a newly retained string; * module map names now use properly formatted external repo labels as names, which makes it more obvious what they are; * the roundtrip in header module compilation flows is avoided. This actually fixes a bug where the missing `@@` prefix for label strings resulted in incorrect or even failing `Label` calls due to main repo labels being resolved relative to the rules_cc repo and external repo labels failing the validation of the `Label` constructor. Closes #633 END_PUBLIC PiperOrigin-RevId: 914893127 Change-Id: I516d7fd41462b0044c504df53e624e96a44850e3
copybara-service Bot
pushed a commit
that referenced
this pull request
May 13, 2026
*** Reason for rollback *** Increases memory usage b/512851570 *** Original change description *** Retain module map names as `Label`s Copybara Import from #633 BEGIN_PUBLIC Retain module map names as `Label`s (#633) With the exception of special module map names, all of them are effectively labels. This commit ensures that they are retained as `Label`s, not label string and thus: * memory usage is reduced for module maps from external repos, whose module map name previously resulted in a newly retained string; * module map names now use properly... *** PiperOrigin-RevId: 914964373 Change-Id: I417bd251d894b2ec7d9837790301abe96dd88fd3
copybara-service Bot
pushed a commit
to bazelbuild/bazel
that referenced
this pull request
May 13, 2026
*** Reason for rollback *** Increases memory usage b/512851570 *** Original change description *** Retain module map names as `Label`s Copybara Import from bazelbuild/rules_cc#633 BEGIN_PUBLIC Retain module map names as `Label`s (#633) With the exception of special module map names, all of them are effectively labels. This commit ensures that they are retained as `Label`s, not label string and thus: * memory usage is reduced for module maps from external repos, whose module map name previously resulted in a newly retained string; * module map names now use properly... *** PiperOrigin-RevId: 914964373 Change-Id: I1ffb91216c6ccb398feb378876cd5e659a3afcb6
Collaborator
|
This causes some of our builds to see memory usage increase by 0.5-1.0%; we're reverting this. If the memory usage improvements this is expected to provide for external uses are significant we can consider having different behavior internally/externally. The @@ bug is probably worth fixing separately but I don't know what the priority on that is. |
Collaborator
Author
|
@lilygorsheneva Are you using separate module maps quite heavily internally? That's the only potential source of additional memory usage I see and I could optimize that case. |
Collaborator
|
I believe we are, for layering check, as well as for low level libraries where we enable clang header modules. |
fmeum
added a commit
to fmeum/rules_cc
that referenced
this pull request
Jul 17, 2026
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 are canonical label strings without the leading '@@' (e.g. 'rules_cc+//pkg:lib'), which Label() fails to parse for targets in external repositories ('invalid package name'), breaking header module compilation for all such targets. The new get_module_map_label() helper either returns file.owner directly or restores the '@@' prefix before parsing. Special module maps (the crosstool module map and ObjC internal module maps) keep their explicitly provided names.
fmeum
added a commit
to fmeum/rules_cc
that referenced
this pull request
Jul 17, 2026
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
With the exception of special module map names, all of them are effectively labels. This commit ensures that they are retained as
Labels, not label string and thus:@@prefix for label strings resulted in incorrect or even failingLabelcalls due to main repo labels being resolved relative to the rules_cc repo and external repo labels failing the validation of theLabelconstructor.