[engine] Reuse stdout buffer for git operations and exec
Since parallel checks are always limited to NumCPU()+2, this limits the
number of stdout buffers created.
This reduce memory usage and fragmentation for exec(). Create three
16kib buffers to reduce initial fragmentation.
This enables running git commands in parallel. For now, only git diff
runs in parallel.
Improvements all around for memory allocation, with significant delta:
name old alloc/op new alloc/op delta
CtxOsExec 137kB ± 0% 70kB ± 0% -49.01% (p=0.004 n=5+6)
CtxOsExec100 9.07MB ± 0% 2.55MB ± 5% -71.85% (p=0.004 n=5+6
Change-Id: If9c92826db94d0bb2193827dfa8fb7d3f7830060
Reviewed-on: https://fuchsia-review.googlesource.com/c/shac-project/shac/+/859576
Reviewed-by: Oliver Newman <olivernewman@google.com>
Commit-Queue: Marc-Antoine Ruel <maruel@google.com>
diff --git a/internal/engine/buffers.go b/internal/engine/buffers.go
new file mode 100644
index 0000000..3f15fb6
--- /dev/null
+++ b/internal/engine/buffers.go
@@ -0,0 +1,57 @@
+// Copyright 2023 The Shac Authors
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package engine
+
+import (
+ "bytes"
+ "sync"
+)
+
+// buffers is the shared buffers across all parallel checks.
+//
+// Fill up 3 large buffers to accelerate the bootstrap.
+var buffers = buffersImpl{
+ b: []*bytes.Buffer{
+ bytes.NewBuffer(make([]byte, 0, 16*1024)),
+ bytes.NewBuffer(make([]byte, 0, 16*1024)),
+ bytes.NewBuffer(make([]byte, 0, 16*1024)),
+ },
+}
+
+type buffersImpl struct {
+ mu sync.Mutex
+ b []*bytes.Buffer
+}
+
+func (i *buffersImpl) get() *bytes.Buffer {
+ var b *bytes.Buffer
+ i.mu.Lock()
+ if l := len(i.b); l == 0 {
+ b = &bytes.Buffer{}
+ } else {
+ b = i.b[l-1]
+ i.b = i.b[:l-1]
+ }
+ i.mu.Unlock()
+ return b
+}
+
+func (i *buffersImpl) push(b *bytes.Buffer) {
+ // Reset keeps the buffer, so that the next execution will reuse the same allocation.
+ b.Reset()
+ i.mu.Lock()
+ i.b = append(i.b, b)
+ i.mu.Unlock()
+}
diff --git a/internal/engine/runtime_ctx_os.go b/internal/engine/runtime_ctx_os.go
index 52c0c24..36298a8 100644
--- a/internal/engine/runtime_ctx_os.go
+++ b/internal/engine/runtime_ctx_os.go
@@ -163,11 +163,15 @@
cmd := s.sandbox.Command(ctx, config)
+ stdout := buffers.get()
+ stderr := buffers.get()
+ defer func() {
+ buffers.push(stdout)
+ buffers.push(stderr)
+ }()
// TODO(olivernewman): Also handle commands that may output non-utf-8 bytes.
- var stdout strings.Builder
- var stderr strings.Builder
- cmd.Stdout = &stdout
- cmd.Stderr = &stderr
+ cmd.Stdout = stdout
+ cmd.Stderr = stderr
var retcode int
// Serialize start given the issue described at sandbox.Mu.
diff --git a/internal/engine/runtime_ctx_scm.go b/internal/engine/runtime_ctx_scm.go
index 8ca2d38..4c0da21 100644
--- a/internal/engine/runtime_ctx_scm.go
+++ b/internal/engine/runtime_ctx_scm.go
@@ -248,10 +248,9 @@
// Mutable. Late initialized information.
mu sync.Mutex
- modified []file // modified files in this checkout.
- all []file // all files in the repo.
- err error // save error.
- b bytes.Buffer // used by run().
+ modified []file // modified files in this checkout.
+ all []file // all files in the repo.
+ err error // save error.
}
func (g *gitCheckout) init(ctx context.Context, root string) error {
@@ -315,13 +314,14 @@
)
}
cmd.Env = g.env
- cmd.Stdout = &g.b
- cmd.Stderr = &g.b
+ b := buffers.get()
+ cmd.Stdout = b
+ cmd.Stderr = b
err := cmd.Run()
// Always make a copy of the output, since it could be persisted. Only reuse
// the temporary buffer.
- out := g.b.String()
- g.b.Reset()
+ out := b.String()
+ buffers.push(b)
if err != nil {
if errExit := (&exec.ExitError{}); errors.As(err, &errExit) {
g.err = fmt.Errorf("error running git %s: %w\n%s", strings.Join(args, " "), err, out)
@@ -457,10 +457,7 @@
if f.action() == "D" {
return make(starlark.Tuple, 0), nil
}
- // TODO(maruel): It should be okay to run these concurrently.
- g.mu.Lock()
o := g.run(ctx, "diff", "--no-prefix", "-C", "-U0", "--no-ext-diff", "--irreversible-delete", g.upstream.hash, "--", f.rootedpath())
- g.mu.Unlock()
if o == "" {
// TODO(maruel): This is not normal. For now fallback to the whole file.
v, err := newLinesWhole(g.checkoutRoot, f.rootedpath())
