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

new ets #72

Closed
wants to merge 3 commits into from
Closed

new ets #72

wants to merge 3 commits into from

Conversation

ruslandoga
Copy link
Contributor

@ruslandoga ruslandoga commented Nov 25, 2023

fixes #71

I'm going to leave some comments inline explaining the changes.

.formatter.exs Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
.tool-versions Show resolved Hide resolved
## 7.0.0 - unreleased

- remove `poolboy` dependency
- improve ETS backend efficiency ~15x
Copy link
Contributor Author

@ruslandoga ruslandoga Nov 25, 2023

Choose a reason for hiding this comment

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

Under load it's actually x40+

x15 (according to Benchee) is when Poolboy is not overloaded

https://github.com/ruslandoga/rate_limit

- improve ETS backend efficiency ~15x
- fix ETS backend overload
- remove global supervisor

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This log is incomplete, I'll add more changes if the approaches in this PR are approved.

Copy link
Contributor

Choose a reason for hiding this comment

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

So if we are going to make 7.0 , I would like to make another braking change. So maybe let’s just merge in master without bumping the version yet ?

Copy link
Contributor

Choose a reason for hiding this comment

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

I would love to see a migrating plan in the changelog from config to the supervisor

max(limit - count, 0),
ms_to_next_bucket,
_created_at = nil,
_updated_at = nil
Copy link
Contributor Author

@ruslandoga ruslandoga Nov 25, 2023

Choose a reason for hiding this comment

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

I searched for Hammer.inspect_bucket on github and it doesn't seem like anyone is using created_at and updated_at values (almost everyone calls this function to get only ms_to_next_bucket). So I think created_at and updated_at can be removed, especially since created_at in ETS backend is incorrect after table pruning (cleanup).

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes but we don’t know how people use it in private repo ?

Copy link
Contributor Author

@ruslandoga ruslandoga Dec 22, 2023

Choose a reason for hiding this comment

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

I suggest dropping these values and reintroducing them back if someone complains. Personally, I don't see a scenario where they are useful, especially with ETS backend which cleans up the keys and makes created_at inaccurate.

Copy link

Choose a reason for hiding this comment

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

@ruslandoga I'd suggest adding a deprecation warning first, and then getting rid of them in the version after. That's how Elixir and core libraries do it.

start_link([])
end
@behaviour Hammer.Backend
@table :hammer_ets_buckets
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd like to make table name configurable.

mix.exs Show resolved Hide resolved
@@ -21,15 +19,14 @@ defmodule Hammer.Mixfile do

def application do
# Specify extra applications you'll use from Erlang/Elixir
[mod: {Hammer.Application, []}, extra_applications: [:logger, :runtime_tools]]
[extra_applications: [:logger]]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It doesn't seem like the project is using :logger, so it can be removed.

@JuneKelly
Copy link
Contributor

Hi! Thanks for opening this PR, and sorry we haven't seen it until now. I'll try to look at this soon, probably over the winter break.

@JuneKelly
Copy link
Contributor

cc @epinault

@epinault
Copy link
Contributor

Wow yes sorry I ll take a look at it this week but same I never got a notification on this either . I ll have to check my settings cause I usually get notified

@@ -1,68 +0,0 @@
defmodule Hammer.Application do
@moduledoc """
Hammer application, responsible for starting the backend worker pools.
Copy link
Contributor

Choose a reason for hiding this comment

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

some of tha doc is still valid here I think like the optionds so we might need to move that to the hammer.ex

Copy link

@egeersoz egeersoz left a comment

Choose a reason for hiding this comment

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

Thank you very much for working on this @ruslandoga! I have only one minor suggestion.

.github/workflows/ci.yml Show resolved Hide resolved
max(limit - count, 0),
ms_to_next_bucket,
_created_at = nil,
_updated_at = nil
Copy link

Choose a reason for hiding this comment

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

@ruslandoga I'd suggest adding a deprecation warning first, and then getting rid of them in the version after. That's how Elixir and core libraries do it.

@ruslandoga
Copy link
Contributor Author

ruslandoga commented Jan 4, 2024

@egeersoz @epinault

Thank you for the comments! I'm going to split this PR into multiple non-breaking ones, some to update CI, some to update ETS backend without the breaking changes, and then a few with the breaking changes.

@ruslandoga ruslandoga closed this Jan 4, 2024
@ruslandoga ruslandoga deleted the new-ets branch January 4, 2024 22:32
@ruslandoga ruslandoga mentioned this pull request Jan 4, 2024
@epinault epinault mentioned this pull request Feb 1, 2024
@ruslandoga
Copy link
Contributor Author

ruslandoga commented May 5, 2024

It seems like no matter where I start: removing application.ex and global supervisor, removing global config and app env lookups, removing poolboy, -- it all results in breaking changes every time :)

And every time I end up with something similar to this:

# instead of config, each backend is defined as a module
defmodule MyApp.RateLimit do
  use Hammer, backend: :ets, table: __MODULE__ # that would be the default table
end

defmodule MyApp.AnotherRateLimit do
  use Hammer, backend: Hammer.Backend.Redis, pool: __MODULE__ # that would be the default pool
end

# each backend can be started separately and be configured at start-up
MyApp.RateLimit.start_link(cleanup_period: :timer.seconds(10))
MyApp.AnotherRateLimit.start_link(url: "...", ...)

# module calls remove the need for runtime backend lookups
{:allow, _} = MyApp.RateLimit.hit("some-key", _scale = :timer.seconds(60), _limit = 3)
1 = MyApp.RateLimit.get("some-key", _scale = :timer.seconds(60), _limit = 3)
MyApp.RateLimit.set("some-key", _scale = :timer.seconds(60), 3)
MyApp.RateLimit.wait("some-key", _scale = :timer.seconds(60), _limit = 3) # = Process.sleep(next_bucket())
MyApp.RateLimit.reset("some-key", _scale = :timer.seconds(60)) # = set(...,...,0)
# etc.

So I'm thinking, would it be OK to just resume this PR and implement this version I always end up with?

@epinault
Copy link
Contributor

sure!

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.

Performance degradation over time with ETS backend
4 participants