feature: enable async sandboxing/digests

[rgrinberg]

These seem to speed up builds in practice

Signed-off-by: Rudi Grinberg me@rgrinberg.com

[Alizter]

Have you got some benchmarks for the speedup?

[rgrinberg]

We do. Let's see if @snowleopard can share them.

[snowleopard]

Why this diff?

[rgrinberg]

Because otherwise the tests aren't deterministic. In particular the ones that rely on --display=short. I'll eventually get rid of that flag in the tests and re-enable this.

[snowleopard]

Are you saying that before this change builds we're somehow fully deterministic? That seems unlikely.

[snowleopard]

I guess they were non-deterministic but perhaps in ways unobservable in current tests. And now non-determinism increases.

[rgrinberg]

Yes, they were deterministic enough for the test output to be reliable. With these toggles enabled, it's no longer the case.

[snowleopard]

Looks good! Internally, we get ~5% speed up on some builds from background sandboxes, and ~4% speed up from background digests (though this one only when writing into shared cache).

[snowleopard]

That builds are becoming less deterministic makes me less excited about changing this default. Though probably still makes sense.

[rgrinberg]

That builds are becoming less deterministic makes me less excited about changing this default. Though probably still makes sense.

Yeah, it's regrettable but I think it's a flaw with our test suite rather than the feature. The execution trace of a build that runs external commands isn't deterministic and we rely on it just because it's easy.

[Alizter]

A comment about the use of --display short in the test suite: It can be useful to quickly see what a build has actually done. In some tests if we just remove the argument then we would have to add ls commands checking targets have been built.

Perhaps we could instead log which targets are built and print them in a deterministic way to get the same effect.

[rgrinberg]

Simplest would be to just sort the output from --display short. But we should still avoid relying on it. Even without this issue, the output from it so too unstable between dune versions. It causes a lot of spurious changes.

[Alizter]

Maybe its my own personal preference, but I prefer it when I make a change and it changes an unrelated test, since it makes me aware of the scope of the changes. When tests continue to pass without notice we have less information about the original change.

[rgrinberg]

That works when your test suite is small, but quickly breaks down when past a certain size. We've had plenty of bugs slip through our test suite just because the changes that introduced the bug included massive test diffs that nobody bothered to read carefully.