Add experimental lazy location expansion for Args - #30291
Draft
fmeum wants to merge 1 commit into
Draft
Conversation
ctx.expand_location(input, targets, lazy = True) (gated behind --experimental_lazy_location_expansion) returns an opaque LazyLocationExpansion instead of an eagerly expanded string. Passing it to Args.add stores a compact recipe in the retained command line: a reference to the original input string (typically an attribute value that is retained by the rule anyway), an int[] of (start, end, mode) triples for the location expression sites, and the resolved artifacts. No string containing artifact paths is created during analysis at all. Paths are rendered when the command line is preprocessed, mirroring LocationExpander.LocationFunction's rendering (callable paths, sorted and deduplicated plural expansions, shell escaping), with exec paths subject to path mapping. Literal segments and unknown functions render directly from the original string, so no substrings are retained either. Validation (unknown labels, empty or multiple expansions for singular functions) still happens during analysis with the same errors as eager expansion, by rendering paths transiently. The name of the main repository's runfiles directory, which $(rlocationpath) rendering needs, is a constant that only depends on the product; it is recorded once when the ConfiguredRuleClassProvider is constructed instead of being stored per expansion, keeping it out of both the instance layout and the retained encoding. Compared to an eagerly expanded string, which retains roughly 36 + length bytes per argument per action, the recipe retains ~36 bytes plus 12 bytes per expression site, independent of path lengths - and is the only form that supports path mapping, which can never apply to eagerly baked path strings. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012hbS4fiUT83VKyVTvxJkjf
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
Member
Collaborator
Author
|
I'm exploring doing this all in starlark over in https://github.com/fmeum/expanders.bzl. The native implementation would be more memory-efficient, but I'll first verify that the overall approach works. |
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
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.
Adds
ctx.expand_location(input, targets, lazy = True), gated behind the new--experimental_lazy_location_expansionflag. Instead of an eagerly expanded string, it returns an opaqueLazyLocationExpansionthat can be passed toArgs.addin place of the string.Motivation
Eagerly expanded location strings bake artifact paths into strings that are retained in the analysis cache for the lifetime of the server (roughly 36 + length bytes per argument per action), and such strings can never be path mapped. The lazy form stores a compact recipe instead: a reference to the original input string (typically an attribute value retained by the rule anyway), an
int[]of (start, end, mode) triples for the location expression sites, and the resolved artifacts — no string containing artifact paths is created during analysis at all, and no substrings of the input are retained either. That's ~36 bytes plus 12 bytes per expression site, independent of path lengths.Design
LocationExpander#expandLazilyscans exactly like#expand, but records site spans and resolved artifacts instead of appending rendered paths. Validation (unknown labels, empty or multiple expansions for singular functions) still happens during analysis with the same error messages, by rendering paths transiently.StarlarkCustomCommandLinegains anEXPANDED_LOCATION_ARG_MARKERencoding. Rendering happens at preprocess time and mirrorsLocationFunction(callable paths, sorted and deduplicated plural expansions, shell escaping), with exec paths going through thePathMapper— lazily expanded locations are thus compatible with--experimental_output_paths=strip.$(rlocationpath)rendering) is a constant that only depends on the product, so it is recorded once when theConfiguredRuleClassProvideris constructed rather than stored per expansion — it appears in neither the instance layout nor the retained encoding.Notes for review
lazyparameter onexpand_locationvs. a separate method) and on recording the runfiles directory inConfiguredRuleClassProvider's constructor.Args.add_joined/map_eachtokens; the native form retains less (no literal substrings, no per-token tuples) and needs no library. Happy to share the detailed memory model and benchmark numbers.🤖 Generated with Claude Code
https://claude.ai/code/session_012hbS4fiUT83VKyVTvxJkjf