Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove generic parameters from Hypercore and Storage #139

Merged
merged 4 commits into from
Jul 4, 2024

Conversation

cowlicks
Copy link
Contributor

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

Replaces generic usage on storage with trait objects
Add dummy field to Hypercore so we can remove it's generic in a seperate
commit
@cowlicks cowlicks changed the title Type erase Remove generic parameters from Hypercore and Storage Jun 29, 2024
@cowlicks
Copy link
Contributor Author

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

@ttiurani
Copy link
Member

ttiurani commented Jul 1, 2024

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?

@ttiurani ttiurani merged commit 16acbff into datrs:master Jul 4, 2024
3 of 6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Dynamic dispatch for storage?
2 participants