Avoid unnecessary map allocation when writing progress

[jsternberg]

MultiWriter would create an unnecessary map allocation when the
Write method was used. The Write method would create a progress
object with the meta field initialized to the meta field of the writer
itself. It then invoked its own WriteRawProgress method which would
see two maps with the metadata and erroneously believe that they were
different and needed to be merged into a single metadata map.

Since this map is initialized with the metadata of the writer before
WriteRawProgress is invoked, this merge was unnecessary and could add
a lot of unnecessary memory allocations during a build.

This changes the MultiWriter.Write method to invoke the private
writeRawProgress which performs the actual write and avoids the
metadata merge.

Fixes #3655.

[tonistiigi]

Good find, but I'm not sure if this is actually related to #3655 . Do you have any measurements to support that? This reduces allocations but I don't see it fixing a memory leak.

[tonistiigi]

In WriteRawProgress, if len(p.meta) == 0 we still make a clone of ps.meta. I wonder if that is intentional or not.

[jsternberg]

@tonistiigi I don't think #3655 is a memory leak and it doesn't seem the original issue referred to it as one. I took a look at the linked issues and I suspect they're unrelated.

I think the problem is MultiProgress just holds a reference to every Progress object it receives until it's done and this can put additional strain on the system/GC if they're not retrievable. That specific user seemed to have 16 GB of allocations which is where I'd expect the system to stall and start to do pause the world GC in a last ditch panic if they're using a system with about 16 GB of memory.

The profile they had pointed to this set of calls as being the result of allocations and the profile also showed these as inuse_space.

----------------------------------------------------------+-------------
      flat  flat%   sum%        cum   cum%   calls calls% + context 	 	 
----------------------------------------------------------+-------------
                                         8256.27MB   100% |   github.com/moby/buildkit/util/progress.(*MultiWriter).Write /src/util/progress/multiwriter.go:68
 8256.27MB 52.68% 52.68%  8256.27MB 52.68%                | github.com/moby/buildkit/util/progress.(*MultiWriter).WriteRawProgress /src/util/progress/multiwriter.go:76

The other potentially problematic line is in writeRawProgress on line 91 because append has a behavior that performs badly with very large slices (which we may want to revisit), but the profile doesn't mention it and the go compiler says it's not inlining the call.

$ go build -gcflags=-m=2 ./util/progress/
util/progress/multiwriter.go:88:6: cannot inline (*MultiWriter).writeRawProgress: unhandled op DEFER

So I think that means the only potential memory allocation is the map allocation. I am a bit concerned that I didn't see any memory allocations from the writeRawProgress method though.

It seems like we initialize the meta for p.meta here:

buildkit/util/progress/multiwriter.go

Line 66 in 4c83fcf

meta: ps.meta,

So p.meta = ps.meta at this point. We then invoke WriteRawProgress which sees that ps.meta isn't empty and then merges it with p.meta, but these two values are the same and both of them probably have length 1. The only place where this type seems to be used in the codebase seems to be on this line:

buildkit/solver/jobs.go

Line 376 in 8ffc03b

mpw: progress.NewMultiWriter(progress.WithMetadata("vertex", dgst)),

Which pretty much guarantees that ps.meta will be at least 1 and trigger this reallocation.

My first thought was to improve the merging logic so if len(p.meta) == 0 we didn't make a clone of ps.meta and then don't initialize meta in the Write method, but I think it's probably more straightforward to just initialize the object like it is now and then avoid the section of code that's responsible for merging the metadata. The method and merging behavior is still useful if we end up going through that pathway, but we should avoid it when we're the ones initializing the Progress object.

But yea you're right that we're missing the case where len(p.meta) == 0. That likely seems unintentional to me.

As an aside, I've also found that many times Go is not the best at doing garbage collections fast enough to avoid the Linux OOM killer. I've had scenarios where we used a lot of memory for small objects, the memory wasn't considered "in-use" and should have been collected, but Go was too slow to free the memory and Linux killed the process with OOM even though there was memory theoretically available. Which might explain some of the issues related to the OOM killer.