internal: switch to boxcar from append-only-vec

[davidbarsky]

Boxcar now uses Loom, and I personally trust Loom/Shuttle-tested libraries more than those that aren't despite their apparent simplicity.

(cc: @ibraheemdev)

[netlify]

✅ Deploy Preview for salsa-rs canceled.

Name	Link
🔨 Latest commit	63441f0
🔍 Latest deploy log	https://app.netlify.com/sites/salsa-rs/deploys/67b3a8492fa4970008564a24

[codspeed-hq]

CodSpeed Performance Report

Merging #688 will not alter performance

_{Comparing davidbarsky:davidbarsky/switch-to-boxcar (63441f0) with master (4b74edf)}

Summary

✅ 9 untouched benchmarks

[MichaReiser]

The perf difference is interesting

[ibraheemdev]

I don't see any relevant changes in the CodSpeed diffs, but I assume the difference is because of #555 (comment). If all write paths for the ingredients vector go through a lock, I can add a push_single_writer method (or a new SPMC split type) to boxcar to avoid the extra unnecessary synchronization if you think it's worth doing, but it sounds like that isn't a huge bottleneck anyways.

[Veykril]

Can we use the type in a qualified manner, that is as boxcar::Vec instead of importing it? Or give the import an alias? I find it confusing to overwrite a prelude type like this

[davidbarsky]

I don't see any relevant changes in the CodSpeed diffs, but I assume the difference is because of #555 (comment). If all write paths for the ingredients vector go through a lock, I can add a push_single_writer method (or a new SPMC split type) to boxcar to avoid the extra unnecessary synchronization if you think it's worth doing, but it sounds like that isn't a huge bottleneck anyways.

Ibraheem is far more of an expert on this than I am, but I'm guessing that that the benchmark benefits substantially from a spinlock, but those benefits evaporate under real, day-to-day conditions. At the very least, I'm extremely skeptical of spinlocks outside a few, very narrow circumstances.

Can we use the type in a qualified manner, that is as boxcar::Vec instead of importing it? Or give the import an alias? I find it confusing to overwrite a prelude type like this

Yeah, I'll refactor usages to be fully-qualified.

[davidbarsky]

Updated as per review. I'm inclined to merge this given that most of the regressions could be explained away by "spinlocks look good in benchmarks".

[ibraheemdev]

We see a similar 4% regression on red-knot incremental performance with this change. That said, I am also inclined to merge because I think it's likely that we can avoid holding the lock during writes to boxcar (I have an idea to make this possible), which would mean we aren't single-writer anymore and the spinlocks could start to matter. I am also fine to hold off until that change happens and we have measurable results, but boxcar's implementation is more robust for future changes, and if the Loom support is important we could go either way.

[davidbarsky]

I think Loom support/tests are sufficiently important from a correctness perspective that I’d prefer a Loom-tested library over a non-Loom tested library.

I’m also extremely interested in seeing your locking changes: I imagine it’d be useful regardless of whether I land this PR or not.

[Veykril]

I'm also in favor of this change despite the small regression

[davidbarsky]

Alright, I've rebased and I'll land this change.