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

Refactor: MovingInterquartileMean #13730

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

Swiftb0y
Copy link
Member

@Swiftb0y Swiftb0y commented Oct 8, 2024

what was initially just supposed to be a cleanup/refactor, mutated into a full blown optimization. This is primarily intended as a cleanup though, and I only implemented the benchmark to show that this was not causing regressions in performance. There is still a little more to be done, but these are some preliminary numbers.
Before:

-----------------------------------------------------------------------------
Benchmark                   Time             CPU   Iterations UserCounters...
-----------------------------------------------------------------------------
BM_insertion/16         17485 ns        16857 ns            1 items_per_second=3.79664M/s
BM_insertion/64         20922 ns        20926 ns            1 items_per_second=12.2336M/s
BM_insertion/512       774264 ns       770344 ns            1 items_per_second=2.65855M/s
BM_insertion/4096    55161004 ns     54684805 ns            1 items_per_second=299.608k/s
BM_insertion/16384  891138337 ns    886767771 ns            1 items_per_second=73.9044k/s

After:

-----------------------------------------------------------------------------
Benchmark                   Time             CPU   Iterations UserCounters...
-----------------------------------------------------------------------------
BM_insertion/16         10458 ns         9738 ns            1 items_per_second=6.57219M/s
BM_insertion/64         16030 ns        15969 ns            1 items_per_second=16.0311M/s
BM_insertion/512       745585 ns       735096 ns            1 items_per_second=2.78603M/s
BM_insertion/4096    40907286 ns     40517411 ns            1 items_per_second=404.369k/s
BM_insertion/16384  644210901 ns    641245320 ns            1 items_per_second=102.201k/s

@Swiftb0y Swiftb0y changed the title Refactor: MovingInterquartileMean optimization Refactor: MovingInterquartileMean refactor Oct 8, 2024
@Swiftb0y Swiftb0y force-pushed the refactor/moving-iqm-optimization branch from a274651 to c35a86c Compare October 8, 2024 21:08
@Swiftb0y
Copy link
Member Author

Swiftb0y commented Oct 8, 2024

So I added yet another optimization (custom replace_sorted algorithm instead of erase+insert) which adds further speedups, especially with realistic IQM sizes.

Edit: Just realized that the Y-Label is not quite right, its "elements processed per second".
Edit2: The error bars denote $\pm1\sigma$ (standard deviation) to show that the results are reasonably significant

MovingIQM Benchmark graph

Please review.

@github-actions github-actions bot added the build label Oct 8, 2024
@Swiftb0y Swiftb0y changed the title Refactor: MovingInterquartileMean refactor Refactor: MovingInterquartileMean Oct 8, 2024
@Swiftb0y Swiftb0y force-pushed the refactor/moving-iqm-optimization branch from 1bc2bb1 to c4ad157 Compare October 9, 2024 09:12
This should also optimize speed and memory usage for small-ish
windows (which is the only usecase right now)
@Swiftb0y Swiftb0y force-pushed the refactor/moving-iqm-optimization branch from c4ad157 to a0fccef Compare October 30, 2024 11:18
@Swiftb0y
Copy link
Member Author

I decided to drop replace_sorted as it introduced quite a lot of extra code without much benefit. This should now quite drastically simplify the code while still providing a speedup.

Comment on lines 38 to 41
// std::queue has no .clear(), so creating a temporary and std::swap is the
// next most elegant solution
std::queue<double> empty;
std::swap(m_queue, empty);
Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't
std::queue<double>().swap(m_queue);
work too? Not tested.

Copy link
Member Author

Choose a reason for hiding this comment

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

good catch, yeah that should work.

@JoergAtGithub
Copy link
Member

LGTM! Thank you!

@Swiftb0y
Copy link
Member Author

just noticed I still had some unpushed commits in my tree, it would be nice if could take a look at those too. Thank you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants