Remove generic parameters from `Hypercore` and `Storage`

cowlicks commented 1 week ago

This removes the generic parameter from Storage, which in turn removes the generic parameter form Hypercore. This has been discussed several times (in discord and in referenced issues) so I decided to create this PR to test the idea out.

All tests pass
No new functionality is added so this does not add new tests
documentation: All this adds is a supertrait, which is documented

Context

This would close #109 and supersede #113

Semver Changes

Breaks comparability

Performance

compilation

The build of the hypercore crate itself was slightly faster ~3 second vs ~4 seconds. Which is expected since this removes monomorphization.

Dependent crates would also probably have faster compilation times, but I have not tested that.

benchmarks

It looks like there was no significant change in performance. I ran cargo bench -F cache on master then on this branch. These were the results:


     Running benches/disk.rs (target/release/deps/disk-3fb308ce5fa1aefb)
slow_call/create_disk   time:   [271.60 µs 279.19 µs 286.45 µs]
                        change: [-16.462% -14.634% -12.494%] (p = 0.00 < 0.05)
                        Performance has improved.

slow_call/write disk    time:   [152.61 µs 155.05 µs 157.58 µs]
                        change: [-0.3795% +1.4127% +3.1796%] (p = 0.13 > 0.05)
                        No change in performance detected.

slow_call/read disk     time:   [27.398 µs 28.185 µs 28.979 µs]
                        change: [-2.8723% -0.0003% +3.1770%] (p = 1.00 > 0.05)
                        No change in performance detected.

slow_call/clear disk    time:   [101.25 ns 116.55 ns 133.87 ns]
                        change: [-35.021% +4.7522% +60.371%] (p = 0.85 > 0.05)
                        No change in performance detected.
Found 11 outliers among 100 measurements (11.00%)
  4 (4.00%) high mild
  7 (7.00%) high severe

     Running benches/memory.rs (target/release/deps/memory-fe94faadab210ef0)
create memory           time:   [28.283 µs 28.327 µs 28.376 µs]
                        change: [-11.992% -11.611% -11.244%] (p = 0.00 < 0.05)
                        Performance has improved.
Found 19 outliers among 100 measurements (19.00%)
  3 (3.00%) low mild
  8 (8.00%) high mild
  8 (8.00%) high severe

write memory            time:   [27.698 µs 27.734 µs 27.773 µs]
                        change: [-6.0768% -5.8230% -5.5992%] (p = 0.00 < 0.05)
                        Performance has improved.
Found 3 outliers among 100 measurements (3.00%)
  1 (1.00%) high mild
  2 (2.00%) high severe

read memory             time:   [4.3834 µs 4.4517 µs 4.5057 µs]
                        change: [-4.7051% -1.3563% +2.0805%] (p = 0.44 > 0.05)
                        No change in performance detected.

clear memory            time:   [39.149 ns 39.615 ns 40.137 ns]
                        change: [-12.839% +1.4443% +19.445%] (p = 0.86 > 0.05)
                        No change in performance detected.
Found 12 outliers among 100 measurements (12.00%)
  4 (4.00%) high mild
  8 (8.00%) high severe

cowlicks commented 1 week ago

cc @Frando @ttiurani @yoshuawuyts @bltavares please let me know what y'all think about this! Thanks

ttiurani commented 1 week ago

Thanks for this!

Given hypercore and hypercore-protocol-rs are very closely connected, in order to really test this, both need to work together.

Can you create a corresponding Draft PR for hypercore-protocol-rs so that cargo test --features js_interop_tests and cargo bench there can be run and tested?

datrs / hypercore