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

Add from_iterator/try_from_iterator for the filters. #60

Merged
merged 2 commits into from
Sep 30, 2023

Conversation

vlovich
Copy link
Contributor

@vlovich vlovich commented Oct 28, 2022

Add a helper to construct using an iterator. I'm not convinced these benchmark results aren't indicative of some kind
of machine setup issue on my end. Can't imagine why there'd be an across-the-board perf improvement like that and
on the previous run I did (before I added the new benchmarks) I saw a mix of improvements and regressions.

    BinaryFuse16/from/500000
                            time:   [19.930 ms 19.993 ms 20.060 ms]
                            change: [-17.320% -8.1279% +1.8364%] (p = 0.17 > 0.05)
                            No change in performance detected.
    
    BinaryFuse32/from/500000
                            time:   [19.161 ms 19.305 ms 19.526 ms]
                            change: [-33.901% -21.972% -9.3098%] (p = 0.01 < 0.05)
                            Performance has improved.
    
    BinaryFuse8/from/500000 time:   [18.473 ms 18.537 ms 18.670 ms]
                            change: [-36.472% -26.878% -17.103%] (p = 0.00 < 0.05)
                            Performance has improved.
    
    Fuse16/from/500000      time:   [29.237 ms 29.529 ms 30.126 ms]
                            change: [-53.901% -34.098% -9.1085%] (p = 0.07 > 0.05)
                            No change in performance detected.
    
    Fuse32/from/500000      time:   [30.281 ms 30.392 ms 30.601 ms]
                            change: [-35.093% -20.946% -6.5932%] (p = 0.03 < 0.05)
                            Performance has improved.
    
    Fuse8/from/500000       time:   [29.309 ms 37.546 ms 45.843 ms]
                            change: [-56.022% -43.412% -23.294%] (p = 0.00 < 0.05)
                            Performance has improved.
    
    Xor16/from/500000       time:   [42.116 ms 44.413 ms 46.674 ms]
                            change: [-44.116% -32.964% -18.902%] (p = 0.00 < 0.05)
                            Performance has improved.
    
    Xor32/from/500000       time:   [42.222 ms 42.648 ms 43.433 ms]
                            change: [-6.5170% +0.2829% +7.5735%] (p = 0.95 > 0.05)
                            No change in performance detected.
    
    Xor8/from/500000        time:   [40.687 ms 41.027 ms 41.439 ms]
                            change: [-43.893% -32.745% -17.619%] (p = 0.00 < 0.05)
                            Performance has improved.

Resolves #59

Copy link
Owner

@ayazhafiz ayazhafiz left a comment

Choose a reason for hiding this comment

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

Yeah, the perf delta is very interesting. I haven't tried this locally but I'll run benchmarks on my machine soon. May be worth checking the perf graph.

Thanks for the PR!

src/bfuse16.rs Outdated Show resolved Hide resolved
@vlovich vlovich force-pushed the add_from_iterator branch 2 times, most recently from d639501 to 756d7b4 Compare November 2, 2022 22:31
src/bfuse16.rs Outdated Show resolved Hide resolved
src/bfuse16.rs Outdated Show resolved Hide resolved
@ayazhafiz
Copy link
Owner

This LGTM other than the comments by @jakelee8 above (thanks @jakelee8 !)

@vlovich
Copy link
Contributor Author

vlovich commented Aug 31, 2023

Sorry for the late reply. Didn't notice this until recently & freed up some time. Review comments addressed.

@ayazhafiz
Copy link
Owner

Thanks @vlovich! Looks like there are a few lint warnings to fix.

Add a helper to construct using an iterator.

BinaryFuse16/from/500000
                        time:   [19.930 ms 19.993 ms 20.060 ms]
                        change: [-17.320% -8.1279% +1.8364%] (p = 0.17 > 0.05)
                        No change in performance detected.

BinaryFuse32/from/500000
                        time:   [19.161 ms 19.305 ms 19.526 ms]
                        change: [-33.901% -21.972% -9.3098%] (p = 0.01 < 0.05)
                        Performance has improved.

BinaryFuse8/from/500000 time:   [18.473 ms 18.537 ms 18.670 ms]
                        change: [-36.472% -26.878% -17.103%] (p = 0.00 < 0.05)
                        Performance has improved.

Fuse16/from/500000      time:   [29.237 ms 29.529 ms 30.126 ms]
                        change: [-53.901% -34.098% -9.1085%] (p = 0.07 > 0.05)
                        No change in performance detected.

Fuse32/from/500000      time:   [30.281 ms 30.392 ms 30.601 ms]
                        change: [-35.093% -20.946% -6.5932%] (p = 0.03 < 0.05)
                        Performance has improved.

Fuse8/from/500000       time:   [29.309 ms 37.546 ms 45.843 ms]
                        change: [-56.022% -43.412% -23.294%] (p = 0.00 < 0.05)
                        Performance has improved.

Xor16/from/500000       time:   [42.116 ms 44.413 ms 46.674 ms]
                        change: [-44.116% -32.964% -18.902%] (p = 0.00 < 0.05)
                        Performance has improved.

Xor32/from/500000       time:   [42.222 ms 42.648 ms 43.433 ms]
                        change: [-6.5170% +0.2829% +7.5735%] (p = 0.95 > 0.05)
                        No change in performance detected.

Xor8/from/500000        time:   [40.687 ms 41.027 ms 41.439 ms]
                        change: [-43.893% -32.745% -17.619%] (p = 0.00 < 0.05)
                        Performance has improved.
@vlovich
Copy link
Contributor Author

vlovich commented Sep 3, 2023

Fixed I think.

@vlovich
Copy link
Contributor Author

vlovich commented Sep 22, 2023

ping @ayazhafiz

@ayazhafiz
Copy link
Owner

Sorry for the delay @vlovich . This looks good, I am merging and will release a patch with some of the latest changes shortly.

@ayazhafiz ayazhafiz merged commit 7b10530 into ayazhafiz:master Sep 30, 2023
7 checks passed
@ayazhafiz
Copy link
Owner

@vlovich , this is available in https://github.com/ayazhafiz/xorf/releases/tag/0.9.0. Thank you so much!

@vlovich vlovich deleted the add_from_iterator branch October 30, 2023 00:42
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.

try_from with an iterator of keys
3 participants