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

Hot path optimizations #25

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

Hot path optimizations #25

wants to merge 2 commits into from

Conversation

hissssst
Copy link

  • Makes custom bucket do binary search for corresponding bucket for value instead of linear search.
  • Slightly improves performance of functions which are executed in event handler by removing unnecessary Access, compiler option for inlining, map destructurisation

float_buckets =
buckets
|> Enum.map(&(&1 * 1.0))
|> Enum.with_index()
Copy link
Contributor

Choose a reason for hiding this comment

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

You can use with_index/2 and do it once, but please double check the Elixir version support.

Copy link
Author

Choose a reason for hiding this comment

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

It is executed in compile time so I wouldn't worry too much about it

@rkallos
Copy link
Owner

rkallos commented Dec 23, 2024

Thank you for your contribution!

This PR made me curious, as I thought that Erlang's function head pattern matching was about as fast as it gets, and that it does binary search on arguments where appropriate, using Erlang's term ordering.

For a small number of buckets (i.e. using Peep.Buckets.PowersOfTen), the current implementation and your implementation have similar performance, or the difference was indistinguishable in my benchmarking.

However, with 1000 buckets (the maximum I was able to stuff into a single module), your implementation's p99 latency is only 1/3rd that of the current implementation, which is a great improvement!

Given that your Peep.Buckets.Custom implementation appears to always be at least as fast as the current implementation, it seems there's no downside to accepting your contribution.

I've fallen a bit behind on reviewing contributions here, but I intend to carefully review and address the current set of open PRs in the coming days.

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.

3 participants