Export: trimming enhancements by DuBento · Pull Request #3530 · thought-machine/please

DuBento · 2026-05-01T14:45:16Z

Enhancements to plz export, moving from a basic target-level trimming (using gc.RewriteFile) to build statement-level trimming, including only the required build rules and subincludes.
For consistency, we format all the exported BUILD files.

Changelog:

Introduced PackageMetadata to track the relationship between BUILD file statements, the targets they generate, and the subincludes they require. Made optional to avoid the overhead for most ops and enabled for the export.
Introduced ScopeMetadata to track object origins (subincluded labels) and statement tracking during the interpreter phase. Made optional to avoid the overhead for most ops and enabled for the export.
Refactored src/export/export.go to enforce better separation of the DefaultExporter (for trimming) and NoTrimExporter.
Added logic to parse BUILD files and selectively write back statements based on whether the generated targets are part of the export set.
Implemented "minimal subinclude" generation, which rewrites subinclude() calls to only include labels actually used by the exported targets.
Update and added some of the e2e to reflect the changes in implementation.
Moved trimming from a GC-based target removal to a full AST statement walk.

toastwaffle · 2026-05-08T14:49:25Z

+		SubrepoName:       subrepo,
+		targets:           map[string]*BuildTarget{},
+		Outputs:           map[string]*BuildTarget{},
+		BuildFileMetadata: newNoopPackageMetadata(),


Why do we always use Noop here?

We default to noop and override if the metadata option is set. Please take a look at the unified NewPackage. Does it make sense?

toastwaffle · 2026-05-08T14:55:57Z

+// RegisterStatement maps a build statement to target in the package.
+func (pkg *Package) RegisterStatement(target *BuildTarget, stmtProvider BuildStatementProvider) {
+	pkg.mutex.Lock()
+	defer pkg.mutex.Unlock()


Why are you only protecting writes with the mutex, and not reads? Go will panic if there is a concurrent read and write.

tbh, I'd expect any locking to be done inside the BuildFileMetadata implementation (which would avoid paying the cost of locking when we're using the Noop implementation). Better still would be to use concurrency-safe datastructures wherever possible (which most likely use locks under the hood anyway, but might not)

I was going to justify the use of *Package level methods for reusing the mutex. I initially enriched the AddTarget logic with a BuildStatement, reusing that lock but eventually separated into different methods.

Read is currently only done synchronously, since the export doesn't (yet) support multi threading. I fully agreed and will migrate BuildFileMetadata to concurrent-safe maps. Thanks for raising this.

Updated. I've used RWMutex instead of dedicated data structures. Mostly because of simplicity and avoiding the performance overhead but also to follow the example of Package and BuildTarget. I can be persuaded in the other direction but I think the parsing is sparse and the operation quick enough that we won't be waiting on the locks that often.

toastwaffle · 2026-05-11T09:24:36Z

+	state     *core.BuildState
+	targetDir string
+
+	exportedTargets map[*core.Package]map[core.BuildLabel]bool


Using a pointer as a key always makes me uncomfortable; can we avoid this?

More generally, could we avoid the nested map? core.BuildLabel includes the package name anyway, right?

We can probably avoid using the pointer has key by using the package label, but we will have to look up in the graph each time. I was using the pointer directly assuming some consistency of no repeated package instances. Should I use the string instead and lookup in the graph each time we want to use it?

The nested map is useful for looping though the exported target per each package (and for efficient verification of visited targets). What's your opinion, should I try to unnest?

toastwaffle · 2026-05-11T15:25:43Z

+		return ""
+	}
+
+	sort.Sort(filteredLabels)


In the interests of making minimal changes to the export, I think we should remove this sort?

Since we use a map to register these labels, the order of insertion is not enforced. Sorting ensures that the output is deterministic. We could possibly retain some of the original order by changing some of the logic in PackageMetadata if you consider this important, however, if we keep the formatting I believe it sorts the subincludes.

toastwaffle · 2026-05-12T08:01:10Z

+	return func() *core.BuildStatement {
+		stmtScope := s
+		for curr := s; curr != nil; curr = curr.callerScope {
+			if curr.pkg != nil && curr.filename == s.pkg.Filename {


Think we need more of a comment here and more in the doc comment to explain this condition and why we don't break once it's true (which I'm guessing is to handle somebody defining a function in a BUILD file? Do we have an export test case for that? And an export test case for statements inside loops and if-statements?).

If I understand this correctly, we're effectively looking for the highest-level function call which is inside a BUILD file (as opposed to calls within other functions)?

Can we unit-test this function and ActiveSubincludes?

Agreed, thanks for calling this out. I've added more in depth comments for both methods. e2e test for a function definition in the same package file was also added.
I've added unit test for both methods but I'm not thrilled about them, let me know if you have any ideas on how to improve. I took the approach of building and linking scopes directly, one potential idea could be to define some custom "native_code", inject in the original scope and use that as a callback when parsing a file. Either approaches don't seem great to me.

I think your test for CurrentBuildStatement is fine, as it's relatively understandable what the test data represents. I'm less sure about the ActiveSubincludes test, which has a lot more logic and it's less clear what's going on.

In general, I'm more in favour of setting up a test repo with BUILD files/defs which actually get parsed, as that's much closer to the public interface to the interpreter/parser. Injecting some custom native code for testing actually seems like quite an elegant solution to me - it would effectively be some sort of assertion function?

We can attempt that. I agree this is very confusing in its current state and I also prefer having a test repo. The idea of having go tests was simple for speed, efficiency and testing with smaller scope but the e2e tests should be test most of the logic. Should I drop these Go tests or attempt the native code injection?

toastwaffle · 2026-05-12T08:04:14Z

 	if f.nativeCode != nil {
 		if f.kwargs {
-			return f.callNative(s.NewScope("<builtin code>", 0), c)
+			return f.callNative(s.NewScope("", 0), c)


Why this change? I think the <builtin code> thing was useful for debugging

I added that in the previous PR, but since that argument is supposed to be a filename I'm not sure it makes sense. If we attempt to open a file with it, it should fail the same as with using "", I just thought it could be misleading but I'm happy to revert this.

toastwaffle · 2026-06-09T10:46:23Z

+	return func() *core.BuildStatement {
+		stmtScope := s
+		for curr := s; curr != nil; curr = curr.callerScope {
+			if curr.pkg != nil && curr.filename == s.pkg.Filename {


I think your test for CurrentBuildStatement is fine, as it's relatively understandable what the test data represents. I'm less sure about the ActiveSubincludes test, which has a lot more logic and it's less clear what's going on.

In general, I'm more in favour of setting up a test repo with BUILD files/defs which actually get parsed, as that's much closer to the public interface to the interpreter/parser. Injecting some custom native code for testing actually seems like quite an elegant solution to me - it would effectively be some sort of assertion function?

toastwaffle · 2026-06-09T10:54:33Z

 	if state.Cache != nil {
 		state.Cache.Shutdown()
 	}
+	if state.RemoteClient != nil {


out of curiosity, why has this moved?

Moved because of KeepParserRunning and having to call this from the main please.go switch case. Is this wrong? We should probably close the remote connection and fallback to local building (are we even building anything for an export?)

toastwaffle

Nearly there!

As ever, comments phrased as questions probably imply a need for code comments

toastwaffle · 2026-07-08T14:09:38Z

 	return int64(bs.Start)
 }

+// hashBuildStatement mixes the Start and End byte coordinates to produce a unique 64-bit hash.


I feel like for amusement we should state that "This does introduce a 4,294,967,296 byte size limit on BUILD files processed by Please"

Also, is it at all concerning that our hash is not uniformly distributed? What's worse - a non-uniform distribution, or doing more work to make it uniform?

In this case it's not really a limit but it will cause a collision, unless I misunderstood what you mean. Either way we are using a small Cmap with 4 shards, it will lookup the last 2 bits of this "hash". The comment is flat out wrong, it doesn't produce a "unique" hash, this is more like a pseudo hash. I'll see if I can find a better implementation for this but I don't think it is worth sinking too much time into this and use it as best effort. Collisions will cause blocked time but an export is not really a performance critical operation.

I've moved to a higher entropy implementation and added a test to validate the pseudo uniformity. I'm looking to prioritise performance over collisions. I'm still tempted to simply do a bitwise xor but this way we add some entropy to the possibly predictable intervals.

peterebden

I have some worries about crossing package responsibilities here - I get there's a lot more information we need to store to support this, and we can work through a bunch of that, but I think some of these changes like wanting to request parses from code post the actual build is a line we shouldn't cross (and I think maybe we don't have to).

… build targets

setting subincludes at package level instead of at target level

- register subinclude statements in the package metadata - filter subincludes label - export all non build_target related statements

this is no longer relevant for the symbol tracking since we mirror the implicit logic for the actual scope symbols.

…arser alive Replaces the fragile background-daemon "KeepParserRunning" parsing logic with a synchronous upfront-parsing design. When "ForceParseEntirePackage" is enabled, we queue and parse all other targets in a visited package exactly once at parse time.

… in ASP

DuBento · 2026-08-28T19:47:13Z

Comments addressed and ready for another review.

peterebden · 2026-08-31T07:30:50Z

+// LabelSet defines a set of labels implemented using a map.
+type LabelSet map[BuildLabel]struct{}


do we really need this type? it's just a map basically, it doesn't add any additional functionality

This is an existing type, it is used in graph.go, package.go and now package_metadata,go, I simply moved it to the build labels file and exported it. I exported it probably because I wanted to use it in the interpreter but I forgot to do so and, in the places the I need a set, I create a map of build labels. I'll unexport the type but I can remove it entirely if you prefer.

peterebden · 2026-08-31T07:34:18Z

+		go func() {
+			defer wg.Done()
+			_ = state.SyncParsePackage(label)
+		}()


use wg.Go for this

peterebden · 2026-08-31T07:34:33Z

+	// Give the waiters time to block on the wait channel
+	time.Sleep(10 * time.Millisecond)


this doesn't guarantee that they do start blocking

I've added some synchronisation to hopefully improve this, but I don't think we can avoid having a sleep. If you have any ideas, or prefer I remove the test, let me know.

peterebden · 2026-08-31T07:36:10Z

 	d.printLines(targets)
-	for _, line := range cli.CurrentBackend.Output() {
-		d.printf("${ERASE_AFTER}%s\n", line)
+	logs := cli.CurrentBackend.Output()


Maybe it's a bit late now but it would have been really nice to have changes like this split from the bulk of the export change here; it'd be easier to reason about a set of logging changes in isolation. I'm not really clear at the moment why you need to do this; I don't especially object to it but it doesn't seem like plz export has any particularly unique output requirements.

peterebden · 2026-08-31T07:46:17Z

+	// to its StatementMetadata. Refer to [StatementMetadata] for more details but this single
+	// mapping tracks the targets produced by the statement, the subincluded labels required for its
+	// interpretation, and other information.
+	statements *cmap.Map[BuildStatement, *StatementMetadata]


do you really need these to be cmaps? They are explicitly optimised for high concurrency but not low overhead, I didn't anticipate generating large numbers of them (in this case two per package).

Would a map-and-mutex not be adequate here? Or, from the comment, I'm a little unclear if the mutex is even required - they can only be written once and I assume you'd just be reading them later during the export operation?

We initially favoured the thread-safe type instead of a shared mutex. I eventually understood that the write phase is single threaded. We maintained the cmaps for consistency but I agree that it is a waste since it results in creating 8 maps + 8 mutexes per package and likely to include few objects in each map.
I believe it would be correct to have no locking whatsoever but I've refactored to include a RWlock for consistency and to be resistant to any future changes (or multi-threaded export). It's better than having cmaps and hopefully with minimal overhead for our current single threaded logic.

peterebden · 2026-08-31T07:49:52Z

+	// The intention is to finds all the subincluded labels required by the package but not used to
+	// generate targets. An example could be a variable declaration that depends on a subincluded value.
+	// We range over all interpreted statements that require any subincluded target. From those, we
+	// filter out the statements that generate targets and any explicit subinclude() statement calls.


Curious why you need this? I'm struggling a bit to see how this links to what's required for export to work

This is required to support the following examples (snippets from tests):

subinclude("//build_defs:versions_build_def") for version, name in VERSIONS.items(): pass # Trimmed during export

for file in glob(["file*.in"]): genrule( name = "target_" + file.removesuffix(".in"), srcs = [file], outs = [file.removesuffix(".in") + ".out"], cmd = "cp $SRCS $OUT", )

Since we are not trimming variables or for headers, we need to determine what else is required by the BUILD file but doesn't necessarily generate a build target.

in the current implementation the mutex is not required but for consistence (and because the overhead should be minimal) we added the support for a shared mutex

…for query metadata

toastwaffle reviewed May 11, 2026

View reviewed changes

toastwaffle reviewed May 12, 2026

View reviewed changes

DuBento changed the title ~~Export: enhance trimming using build statement metadata~~ Export: trimming enhancements Jun 5, 2026

toastwaffle reviewed Jun 9, 2026

View reviewed changes

DuBento force-pushed the export-from-package-metadata branch 4 times, most recently from f6fbf1a to 7152109 Compare June 17, 2026 10:01

toastwaffle requested changes Jul 11, 2026

View reviewed changes

peterebden requested changes Aug 13, 2026

View reviewed changes

Comment thread src/core/state.go Outdated

Comment thread src/core/state.go Outdated

Comment thread src/core/package_metadata.go Outdated

peterebden reviewed Aug 13, 2026

View reviewed changes

Comment thread test/export/please_export_e2e_test.build_defs Outdated

duarte added 18 commits August 28, 2026 20:24

Package level metadata that includes a mapping of build statements to…

8d0ecc4

… build targets

export using BuildFileMetadata logic

c91668c

register caller scope for callstack-like traversal

1276403

bufio writer for build statements

836fff5

rewrite export logic into explicit flow

a505981

export targets related to build statement

d3e67bc

enrich target with subincludes by looping though all scopes

d09e7d0

separate buildstmt register from adding target

1fb4f18

select and write subincludes

b0c3c1e

setting subincludes at package level instead of at target level

skip statement for preloaded subincludes

3abc3cd

test: trim subincludes

68b61e0

rename file

aeb2e4a

use slices.collect and sort interface

1966979

rewrite export with 2 concrete interfaces: default and notrim

50110d3

notrim: full copy BUILD file and visit sources

65fd873

double new lines between targets

82f9587

suppress diff output when enforcing repo differences

00f617f

export by filtering the original BUILD file

47ab394

- register subinclude statements in the package metadata - filter subincludes label - export all non build_target related statements

duarte added 20 commits August 28, 2026 20:33

State: test for logBuildError

2c1644c

export test formatting

beef2ac

Fix if trim test: declared interpreted statements.

30a5ee5

getOrParse safety timeout

777b1f1

fix infinite counter

5632e13

address query.metadata comments

3bf3179

strip symbols

0931003

s/CleanUp/Cleanup

1cbfd18

s/unneded/unneeded

e6ff712

standardise use of "unused" in tests

ab48c30

comments for interpreter.go

7f0bdfc

test: target variable trimming

a081324

revert: track preloaded in interpreter

229ea4f

this is no longer relevant for the symbol tracking since we mirror the implicit logic for the actual scope symbols.

fix: add new lines at the end of files

10b3f05

revert one time variable

23b4f99

test: move complex go test to e2e

7ba7f18

lint: plz fmt

585d95a

fix: wrong reference to unused lib in e2e test

d32f1e6

refactor: decouple buildStatement from core by implementing interface…

7552be7

… in ASP

DuBento force-pushed the export-from-package-metadata branch from 3f140a1 to 7552be7 Compare August 28, 2026 19:42

go lint

ef3bd43

peterebden reviewed Aug 31, 2026

View reviewed changes

duarte added 6 commits September 2, 2026 17:10

unexport labelset

9b8ca27

refactor: use RWmutex instead of cmaps for package metadata

4f7c136

in the current implementation the mutex is not required but for consistence (and because the overhead should be minimal) we added the support for a shared mutex

refactor: introduce struct TrackedStatement instead of sorted slices …

0f073e4

…for query metadata

test: trimming of variables that include targets

0ae4949

test: refactor build error test for better consistency

2c4ae9f

refactor: subincludeArgs as iterator

ff407e4

		// LabelSet defines a set of labels implemented using a map.
		type LabelSet map[BuildLabel]struct{}

		// Give the waiters time to block on the wait channel
		time.Sleep(10 * time.Millisecond)

Uh oh!

Conversation

DuBento commented May 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

toastwaffle left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

DuBento commented May 1, 2026 •

edited

Loading