Vector Clock Feature Flag

[dylanjwolff]

This PR adds a feature flag for vector clocks to boost Shuttle's testing throughput when they are not needed.

This feature flag is disabled by default in production, but always enabled for tests because many tests leverage vector clocks for assertions to check correctness. In the micro-benchmarks for the Shuttle repository, this results in performance gains of 5-25%:

criterion.tar.gz

When disabled, the VectorClock struct is replaced by an empty struct for which all operations are i.e. a no-op.

---

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

[sarsko]

My suggestion is to remove this and cfg all usages of VectorClock

[dylanjwolff]

it would be nice to not need a dummy implementation, but the issue with doing this is that there are other structs that have VectorClock's (e.g.

shuttle/shuttle/src/future/batch_semaphore.rs

Line 69 in 41ae7ba

last_acquire: VectorClock,

) so the blast radius of this change is quite large and would result in the codebase being littered with #[cfg...]'s.

the dummy implementation keeps this PR quite small and gives us probably 99% of the performance benefits

[sarsko]

It is possible to do benching without vector clocks by introducing a feature (eg. bench-no-vector-clocks) and then gating vector clocks on all(any(test, feature = "vector-clocks"), not(feature = "bench-no-vector-clocks")

[dylanjwolff]

Yup, this would allow you to run the benchmarks without vector clocks by enabling the feature at the command line rather than commenting this line out, which I agree is better. I can add it, but I am getting worried that this setup is a little counter-intuitive (feature is disabled by default, but always enabled for tests and benchmarks unless you use this other feature flag).

I'm starting to think it might be better to enable vector clocks by default, add it as a "required feature" for tests, and add a gated compile error to prevent ever using cargo test without the feature. I think this is slightly more intuitive: if the feature is enabled its enabled everywhere and if it is disabled, it is disabled everywhere (but should result in compile errors for testing). For example:

f9d5ced

Sorry for the back and forth on this!

[sarsko]

I think default enabling vector clocks (or most features tbh) is a mistake:

1. It is a "poweruser" feature with runtime performance costs.
2. The default offerings should be sensible. This is not a sensible default (noone has ever used it).
3. The standard recommendation will be "oh and yeah, disable this feature" which is just bad UX.
4. Feature enablement is additive. Having it as a default makes accidental opt-in the standard, which is a pain for projects which span multiple packages.
5. One should opt-in to the functionality one wants, not opt-out of the functionality one does not want.