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

SegmentCache #20

Draft
wants to merge 10 commits into
base: mccolljr/const_generics
Choose a base branch
from
Draft

Conversation

cehteh
Copy link
Contributor

@cehteh cehteh commented Jun 10, 2023

WIP! caching segments should improve performance considerably. integration and benching follows.

* make SegVec const-generic

* SegVec::new can't be const because smallvec's new is non-const

* Fix a bug where the truncate method improperly accounted for factor

* Make some simple functions inline
Copy link
Owner

@mccolljr mccolljr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks quite nice so far. Just a couple thoughts based on a first pass

Comment on lines +1209 to 1310
#[cfg(test)]
fn checked_log2_floor(v: usize) -> Option<u32> {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this is no longer used by the implementation, it is okay to remove it and the associated test.

Comment on lines +17 to +24
/// Called by ctors to assert that the configuration is valid.
///
/// Some `MemConfig` implementations may put constraints on (const generic)
/// parameters. Currently it is impossible to assert these at compile time in stable
/// rust. In debug builds we check these constraints when a `SegVec` uses some
/// config. The three shipped `MemConfig` implementations check here that the 'FACTOR' is
/// not zero.
fn debug_assert_config();
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there are some stable ways to assert things about const generics now.

This is one example (stolen from a thread in an issue on static_assertions):

https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=98221c637c7195f1fc530cc51d3848f8

If you feel up to it, I think it would be better to have these assertions happen at compile time... especially given that the (current) assertions are simple, just checking the FACTOR value against 0.

@cehteh
Copy link
Contributor Author

cehteh commented Jun 10, 2023 via email

* add mem_config.rs for different allocation policies

* comment wording

* The MemConfig impls must be public

* add 'debug_assert_config' for validating config parameters

* integrate MemConfig into SegVec

This removes the 'capacity' member for now, it might be added back in future or be part of the
planned 'cache' feature. Benchmarking will show.

* fixing few clippy lints (some remain, to be addressed later)

* split 'reserve()' into a hot and a cold path and add capacity back

This gives a *significant* performance improvement.

* before
  * push 10k values with default growth factor
    * time:   [133.64 µs 133.96 µs 134.29 µs]
  * push 10k values with large growth factor
    * time:   [81.332 µs 81.857 µs 82.335 µs]

* after
  * push 10k values with default growth factor
    * time:   [79.874 µs 80.206 µs 80.643 µs]
  * push 10k values with large growth factor
    * time:   [46.552 µs 46.704 µs 46.826 µs]

* improve benchmarks

* Increase the loop to 1Mio (from 10k)
* Add benchmars for
  * std Vec to have something to compare to
  * Linear/Proportional/Exponential with FACTOR 32 to compare between them
not integrated yet
* Improves 'reserve()' again while at it.
* only 'push_cached()' for testing
* add cached benchmarks

We are getting closer to std Vec speed!
@cehteh
Copy link
Contributor Author

cehteh commented Jun 12, 2023

I updated the benchmarks (not committed yet) and want to share some observations:

  1. https://public.pipapo.org/segvec_bench/criterion/push%20values/report/index.html
    These push() 100k values.
  • In most cases we are less than 2x slower than std Vec which is pretty good
  • Linear<1> is insanely slow, disabled that in the benchmark.
  • Linear<1024> is the fastest.
  • The Proportional math is very expensive.
  • Caches work but the gains at push() are marginal except for the odd Proportional case. Still slower than Linear<1024>
  1. https://public.pipapo.org/segvec_bench/criterion/access_ordered/report/index.html
    This is just a loop calling get(0..N)

    • std Vec is significantly faster
    • Linear<1024> is fastest because of the simplest math
    • Not cached SegVec is more than a magnitude slower than std Vec
    • Caching works well but is slower than Linear<1024>
  2. https://public.pipapo.org/segvec_bench/criterion/access_semirandom/report/index.html
    This is get(index) where index is randomly modified to +/-500

    • SegVec with default (Exponential<1>) is more than 3 times slower than Vec
    • Caching improves it considerably
    • Linear<1024> is still faster than the cached Exponential<1>
  3. https://public.pipapo.org/segvec_bench/criterion/access_random/report/index.html
    This is totally random access

    • SegVec with defaults performs bad
    • Caching is even worse (not surprising)
    • Linear<1024> is close to the std Vec and pretty fast

Conclusion

Proportional is too slow, needs better math, maybe something that is only 'roughly' proportional (I have no actual idea about that).

While the caching idea works, it's gains are not as big as I hoped for and it is always outperformed by Linear<1024>.

I will scrap the SegmentCache.

  • For speed use 'Linear'.
  • When memory is scare then use 'Exponential'
  • Don't use 'Proportional' unless it becomes improved. (scrap it too?)

https://public.pipapo.org/segvec_bench/criterion/report/

@cehteh
Copy link
Contributor Author

cehteh commented Jul 6, 2023

This might closed now, the code exists as proof-of-concept. It could be reasonable to revive this idea when (cacheable) fast indexing on Proportional or Exponential is required. Still chosing Linear would likely be the better approach.

@cehteh cehteh mentioned this pull request Sep 5, 2023
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.

2 participants