Final refactor, tests pass

src/build/build_step.go

@@ -74,16 +74,13 @@ func Build(state *core.BuildState, target *core.BuildTarget, remote bool) error
 			log.Errorf("Failed to remove outputs for %s: %s", target.Label, err)
 		}
 		target.SetState(core.Failed)
-		target.FinishBuild()
 		return err
 	}
 	if remote {
 		successfulRemoteTargetBuildDuration.WithLabelValues(metrics.CILabel).Observe(float64(time.Since(start).Milliseconds()))
 	} else {
 		successfulLocalTargetBuildDuration.WithLabelValues(metrics.CILabel).Observe(float64(time.Since(start).Milliseconds()))
 	}
-	// Mark the target as having finished building.
-	target.FinishBuild()
 	return nil
 }
 

src/build/build_step_stress_test.go

@@ -33,10 +33,10 @@ func TestBuildLotsOfTargets(t *testing.T) {
 	pkg := core.NewPackage("pkg")
 	state.Graph.AddPackage(pkg)
 
+	targets := []core.BuildLabel{}
 	for i := 1; i <= size; i++ {
-		addTarget(state, i)
+		targets = append(targets, addTarget(state, i).Label)
 	}
-	state.TaskDone() // Initial target adding counts as one.
 
 	results := state.Results()
 	// Consume and discard any results
@@ -47,14 +47,13 @@ func TestBuildLotsOfTargets(t *testing.T) {
 		}
 	}()
 
-	plz.RunHost(nil, state)
+	plz.RunHost(targets, state)
 }
 
 func addTarget(state *core.BuildState, i int) *core.BuildTarget {
 	// Create and add a new target, with a parent and a dependency.
 	target := core.NewBuildTarget(label(i))
 	target.IsFilegroup = true // Will mean it doesn't have to shell out to anything.
-	target.SetState(core.Active)
 	target.Test = new(core.TestFields)
 	state.Graph.AddTarget(target)
 	if i <= size {
@@ -68,9 +67,6 @@ func addTarget(state *core.BuildState, i int) *core.BuildTarget {
 				log.Info("Adding dependency %s -> %s", target.Label, dep)
 				target.AddDependency(dep)
 			}
-		} else {
-			// These are buildable now
-			state.QueueTarget(target.Label, core.OriginalTarget, false, core.ParseModeNormal)
 		}
 	}
 	return target
@@ -101,12 +97,8 @@ type fakeParser struct {
 	PostBuildFunctions buildFunctionMap
 }
 
-func (fake *fakeParser) RegisterPreload(core.BuildLabel) error {
-	return nil
-}
-
 // ParseFile stub
-func (fake *fakeParser) ParseFile(pkg *core.Package, label, dependent *core.BuildLabel, mode core.ParseMode, fs iofs.FS, filename string) error {
+func (fake *fakeParser) ParseFile(pkg *core.Package, label, dependent *core.BuildLabel, fs iofs.FS, filename string) error {
 	return nil
 }
 
@@ -125,7 +117,7 @@ func (fake *fakeParser) Init(state *core.BuildState) {
 }
 
 // ParseReader stub
-func (fake *fakeParser) ParseReader(pkg *core.Package, r io.ReadSeeker, label, dependent *core.BuildLabel, mode core.ParseMode) error {
+func (fake *fakeParser) ParseReader(pkg *core.Package, r io.ReadSeeker, label, dependent *core.BuildLabel) error {
 	return nil
 }
 

src/build/build_step_test.go

@@ -609,12 +609,8 @@ func (*mockCache) Shutdown()                      {}
 type fakeParser struct {
 }
 
-func (fake *fakeParser) RegisterPreload(core.BuildLabel) error {
-	return nil
-}
-
 // ParseFile stub
-func (fake *fakeParser) ParseFile(pkg *core.Package, label, dependent *core.BuildLabel, mode core.ParseMode, fs iofs.FS, filename string) error {
+func (fake *fakeParser) ParseFile(pkg *core.Package, label, dependent *core.BuildLabel, fs iofs.FS, filename string) error {
 	return nil
 }
 
@@ -632,7 +628,7 @@ func (fake *fakeParser) NewParser(state *core.BuildState) {
 }
 
 // ParseReader stub
-func (fake *fakeParser) ParseReader(pkg *core.Package, r io.ReadSeeker, label, dependent *core.BuildLabel, mode core.ParseMode) error {
+func (fake *fakeParser) ParseReader(pkg *core.Package, r io.ReadSeeker, label, dependent *core.BuildLabel) error {
 	return nil
 }
 

src/build/incrementality_test.go

@@ -111,6 +111,7 @@ var KnownFields = map[string]bool{
 	"mutex":                  true,
 	"dependenciesRegistered": true,
 	"finishedBuilding":       true,
+	"ModifiedByCallback":     true,
 
 	// Used to save the rule hash rather than actually being hashed itself.
 	"RuleHash": true,

src/cmap/cerrmap.go

@@ -1,5 +1,9 @@
 package cmap
 
+import (
+	"context"
+)
+
 // A Limiter is the interface that we use to release/acquire workers while waiting.
 type Limiter interface {
 	Acquire()
@@ -32,13 +36,6 @@ func (m *ErrMap[K, V]) Add(key K, val V) bool {
 	return m.m.Add(key, errV[V]{Val: val})
 }
 
-// AddOrGet either adds a new item (if the key doesn't exist) or gets the existing one.
-// It returns true if the item was inserted, false if it already existed (in which case it won't be inserted)
-func (m *ErrMap[K, V]) AddOrGet(key K, f func() V) (V, bool, error) {
-	v, present := m.m.AddOrGet(key, func() errV[V] { return errV[V]{Val: f()} })
-	return v.Val, present, v.Err
-}
-
 // Set is the equivalent of `map[key] = val`.
 // It always overwrites any key that existed before.
 func (m *ErrMap[K, V]) Set(key K, val V) {
@@ -60,7 +57,7 @@ func (m *ErrMap[K, V]) Get(key K) (V, error) {
 // GetOrSet returns the value if set, or an error if one has been set.
 // If nothing has been set for the key, it runs the given function to generate the value and then sets it.
 func (m *ErrMap[K, V]) GetOrSet(key K, f func() (V, error)) (V, error) {
-	v, wait, first := m.m.GetOrWait(key)
+	v, wait, first := m.m.getOrWait(key)
 	if v.Err != nil {
 		return v.Val, v.Err
 	} else if first {
@@ -79,6 +76,32 @@ func (m *ErrMap[K, V]) GetOrSet(key K, f func() (V, error)) (V, error) {
 	return v.Val, v.Err
 }
 
+// GetOrSetCtx is like GetOrSet but accepts a context that can be cancelled.
+func (m *ErrMap[K, V]) GetOrSetCtx(ctx context.Context, key K, f func() (V, error)) (V, error) {
+	v, wait, first := m.m.getOrWait(key)
+	if v.Err != nil {
+		return v.Val, v.Err
+	} else if first {
+		val, err := f()
+		m.m.Set(key, errV[V]{Val: val, Err: err})
+		return val, err
+	} else if wait != nil {
+		if m.l != nil {
+			// Release the limiter for the duration we're waiting
+			m.l.Release()
+			defer m.l.Acquire()
+		}
+		select {
+		case <-wait:
+			return m.Get(key)
+		case <-ctx.Done():
+			var v V
+			return v, ctx.Err()
+		}
+	}
+	return v.Val, v.Err
+}
+
 // Range calls f for each key-value pair in the map.
 // No particular consistency guarantees are made during iteration.
 func (m *ErrMap[K, V]) Range(f func(key K, val V)) {

src/cmap/cmap.go

@@ -53,12 +53,6 @@ func (m *Map[K, V]) Add(key K, val V) bool {
 	return m.shards[m.hasher(key)&m.mask].Set(key, val, false)
 }
 
-// AddOrGet either adds a new item (if the key doesn't exist, calling the given function to create it) or gets the existing one.
-// It returns true if the item was inserted, false if it already existed (in which case it won't be inserted)
-func (m *Map[K, V]) AddOrGet(key K, f func() V) (V, bool) {
-	return m.shards[m.hasher(key)&m.mask].LazySet(key, f)
-}
-
 // Set is the equivalent of `map[key] = val`.
 // It always overwrites any key that existed before.
 func (m *Map[K, V]) Set(key K, val V) {
@@ -67,21 +61,15 @@ func (m *Map[K, V]) Set(key K, val V) {
 
 // Get returns the value corresponding to the given key, or its zero value if the key doesn't exist in the map.
 func (m *Map[K, V]) Get(key K) V {
-	v, _, _ := m.shards[m.hasher(key)&m.mask].Get(key)
-	return v
+	return m.shards[m.hasher(key)&m.mask].Get(key)
 }
 
 func (m *Map[K, V]) Contains(key K) bool {
 	return m.shards[m.hasher(key)&m.mask].Contains(key)
 }
 
-// GetOrWait returns the value or, if the key isn't present, a channel that it can be waited
-// on for. The caller will need to call Get again after the channel closes.
-// If the channel is non-nil, then val will exist in the map; otherwise it will have its zero value.
-// The third return value is true if this is the first call that is awaiting this key.
-// It's always false if the key exists.
-func (m *Map[K, V]) GetOrWait(key K) (val V, wait <-chan struct{}, first bool) {
-	return m.shards[m.hasher(key)&m.mask].Get(key)
+func (m *Map[K, V]) getOrWait(key K) (val V, wait <-chan struct{}, first bool) {
+	return m.shards[m.hasher(key)&m.mask].GetOrWait(key)
 }
 
 // Values returns a slice of all the current values in the map.
@@ -138,32 +126,19 @@ func (s *shard[K, V]) Set(key K, val V, overwrite bool) bool {
 	return true
 }
 
-// LazySet is like Set but calls the given function to construct the object only if needed.
-// It also returns the value that is now set in the map (whether overwritten or not).
-func (s *shard[K, V]) LazySet(key K, f func() V) (V, bool) {
-	s.l.Lock()
-	defer s.l.Unlock()
-	if existing, present := s.m[key]; present {
-		if existing.Wait == nil {
-			return existing.Val, false // already added
-		}
-		// Hasn't been added, but something is waiting for it to be.
-		v := f()
-		s.m[key] = awaitableValue[V]{Val: v}
-		close(existing.Wait)
-		existing.Wait = nil
-		return v, true
-	}
-	v := f()
-	s.m[key] = awaitableValue[V]{Val: v}
-	return v, true
+// get returns the value for a key, or its zero value if it isn't present.
+// Unlike Get it never inserts anything, so it's safe for callers that only want to read.
+func (s *shard[K, V]) Get(key K) V {
+	s.l.RLock()
+	defer s.l.RUnlock()
+	return s.m[key].Val
 }
 
 // Get returns the value for a key or, if not present, a channel that it can be waited
 // on for.
 // Exactly one of the target or channel will be returned.
 // The third value is true if it is the first call that is waiting on this value.
-func (s *shard[K, V]) Get(key K) (val V, wait <-chan struct{}, first bool) {
+func (s *shard[K, V]) GetOrWait(key K) (val V, wait <-chan struct{}, first bool) {
 	s.l.RLock()
 	if v, ok := s.m[key]; ok {
 		s.l.RUnlock()

src/cmap/cmap_test.go

@@ -26,47 +26,38 @@ func TestMap(t *testing.T) {
 	assert.Equal(t, []int{5, 7}, vals)
 }
 
+// TestWait covers the awaiting primitive directly; it's only reachable through ErrMap now,
+// but it's the bit with the interesting concurrency so it's worth pinning down here.
 func TestWait(t *testing.T) {
 	m := New[int, int](DefaultShardCount, hashInts)
-	v, ch, first := m.GetOrWait(5)
+	v, ch, first := m.getOrWait(5)
 	assert.Equal(t, 0, v) // Should be the zero value
 	assert.True(t, first) // We're the first to request it
 	go func() {
 		m.Set(5, 7)
 	}()
 	<-ch
-	v, ch, first = m.GetOrWait(5)
+	v, ch, first = m.getOrWait(5)
 	assert.Nil(t, ch)
 	assert.Equal(t, 7, v)
 	assert.False(t, first)
 }
 
+func TestGetDoesntInsert(t *testing.T) {
+	m := New[int, int](DefaultShardCount, hashInts)
+	assert.Equal(t, 0, m.Get(5))
+	// A failed lookup must not leave an entry behind; anything that later tries to set this key
+	// would find something already waiting on it and never get to do the work.
+	assert.False(t, m.Contains(5))
+}
+
 func TestReAdd(t *testing.T) {
 	m := New[int, int](DefaultShardCount, hashInts)
 	assert.True(t, m.Add(5, 7))
 	assert.False(t, m.Add(5, 7))
-	v, ch, first := m.GetOrWait(5)
-	assert.Nil(t, ch)
-	assert.Equal(t, 7, v)
-	assert.False(t, first)
+	assert.Equal(t, 7, m.Get(5))
 	m.Set(5, 8)
-	v, ch, first = m.GetOrWait(5)
-	assert.Nil(t, ch)
-	assert.Equal(t, 8, v)
-	assert.False(t, first)
-}
-
-func TestAddOrGet(t *testing.T) {
-	m := New[int, int](DefaultShardCount, hashInts)
-	x, inserted := m.AddOrGet(5, func() int { return 7 })
-	assert.True(t, inserted)
-	assert.Equal(t, 7, x)
-	x, inserted = m.AddOrGet(5, func() int { return 8 })
-	assert.False(t, inserted)
-	assert.Equal(t, 7, x)
-	x, inserted = m.AddOrGet(8, func() int { return 9 })
-	assert.True(t, inserted)
-	assert.Equal(t, 9, x)
+	assert.Equal(t, 8, m.Get(5))
 }
 
 func TestShardCount(t *testing.T) {
@@ -91,10 +82,7 @@ func TestResize(t *testing.T) {
 				m.Set(i, i)
 			}
 			for i := 0; i < n; i++ {
-				v, ch, first := m.GetOrWait(i)
-				assert.Equal(t, i, v, "Key %d appears to be not set or set incorrectly", i)
-				assert.Nil(t, ch)
-				assert.False(t, first)
+				assert.Equal(t, i, m.Get(i), "Key %d appears to be not set or set incorrectly", i)
 			}
 		})
 	}