Optimize testee memory allocation

[josharian]

As a data point, before this PR, fuzzing github.com/dvyukov/go-fuzz-corpus/png maxes out on my machine at around 16k execs/sec. After this PR, that increases to about 26k execs/sec.

Please review carefully; the "avoid the giant buffer" commit in particular is subtle.

[josharian]

After this, https://go-review.googlesource.com/c/go/+/164960 and https://go-review.googlesource.com/c/go/+/164961 will also help.

[dvyukov]

You are saying this does not allocate? That's because... we store stdoutPipe as concrete type rather than interface?

[dvyukov]

But this should cause stack growth and copy, right? As far as I remember goroutines used to start with 512b stack.
Stacks are not garbage collected, but that still can produce some overhead.
Could you test a fuzz function that does not produce any output with this change versus replacing this whole goroutine with t.outputC <- nil? If that shows any noticeable difference then we could start with a small read buffer as well.

[dvyukov]

We probably could avoid copying the data as well, but I am not sure it matters (if the main intention is to avoid the "no output" case).

[dvyukov]

After this, https://go-review.googlesource.com/c/go/+/164960 and https://go-review.googlesource.com/c/go/+/164961 will also help.

Nice! I like efficient standard library!

[dvyukov]

Have you considered plumbing sync.Pool somewhere here?
I understand that we return the output to caller. But if we restart the binary after 10K iters, in that case we could recycle the buffer.
If we actually crashed, we have a choice of either giving the buffer to caller, or making a smaller copy and recycling the original large buffer.
Just thinking maybe there are some other low hanging fruits here. I have not seen the profiles, though.

Also, potentially we could tune the 1MB buffer size and 10K iters restarts. These were chosen pretty much by a fair dice roll very early in the development.

There is also another interesting option of not collecting output at all... unless the binary has crashed. Just as we run sonar binary only when we actually need its results, we could collect output only when triaging/minimizing a crash.

Another interesting possibility is to enlarge pipe buffer then just read the buffer size with ioctl(FIONREAD) and only if we see that the buffer is close to full, we start doing anything. Otherwise we can simply keep it in the pipe itself. That may have some portability issues, though.

[josharian]

Have you considered plumbing sync.Pool somewhere here?

I didn't use sync.Pool here because it tends to not work well for managing a few large allocations (where large is measured relative to heap size). Discussion at golang/go#22950. Ideally we could fix golang/go#29696 and golang/go#18802 and then build a sync.Pool replacement that was better suited to that kind of use. :)

(Also interesting is that the real cost here isn't so much the allocation as the memclr.)

However, re-reading your comments, it occurs to me that we can just hang a giant buffer off of worker and then have each new testee use it in turn. We wouldn't even need to clear it.

I'll try that approach and report back.

Also, potentially we could tune the 1MB buffer size and 10K iters restarts.

If the worker approach sketched above works, then the 1MB buffer will serve as useful heap ballast (see golang/go#19839 (comment) and golang/go#23044).

I do plan to revisit the 10k iters. I want to do two things:

• Add a way for a Fuzz function to only execute once. Short term I was thinking a go-fuzz flag. Medium term I'd like to migrate to Fuzz functions taking an object rather than a byte slice, and that object could have an "I can only run once" method on it, which moves that information to the code, where it belongs. (I'll open an issue to discuss this.)

• Have go-fuzz monitor the child process, e.g. memory usage or average execution time over some sliding window, and make a decision dynamically about when a restart is required.

[josharian]

I'll try that approach and report back.

Done. Much nicer. Thanks for pushing back.

Add a way for a Fuzz function to only execute once.

#218

make a decision dynamically about when a restart is required.

Future work. :)

[dvyukov]

Eeee
this buffer is returned from TestBinary.test and then leaked to crasherQueue
we can't just do this
we can do something like this, but we can reuse the buffer only on periodic testee restart, on crashes we can either handoff the buffer to caller (assuming crashes are not so frequent, or if they are perf does not matter) or make a copy of the used part of the buffer and then reuse the original buffer

[josharian]

I apologize, I don't follow. A few lines down, around line 215, I see:

		trimmed := make([]byte, filled)
		copy(trimmed, data)
		t.outputC <- trimmed

So it seems like data/buffer doesn't leak. What am I missing?

[dvyukov]

You are right!
I assumed we send the buffer to the caller as-is. I guess I did it to not pin the large buffer... perhaps...