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

Reapply #3878 + bugfix hit rate tracking #3891

Merged
merged 5 commits into from
Mar 14, 2024
Merged

Conversation

aerosol
Copy link
Member

@aerosol aerosol commented Mar 12, 2024

Changes

This PR re-applies #3878 with one extra commit on top of it: 6019ebe

Previous approach wrongly assumed we can detect if an item was warm or cold on get_or_store.
Ideally the telemetry (or any functionally equivalent approach) would have been implemented in con_cache - I guess we'll find out. Luckily for us, con_cache exposes low-level Operations interface for which I'm very grateful :)

Tests

  • Automated tests have been added
  • This PR does not require tests

Changelog

  • Entry has been added to changelog
  • This PR does not make a user-facing change

Documentation

  • Docs have been updated
  • This change does not need a documentation update

Dark mode

  • The UI has been tested both in dark and light mode
  • This PR does not change the UI

@aerosol aerosol requested review from zoldar and ruslandoga March 12, 2024 19:15
@aerosol aerosol force-pushed the reapply-cache-change branch from 69a32c6 to 6019ebe Compare March 12, 2024 19:38
@aerosol aerosol changed the title Reapply cache change Reapply #3878 + bugfix hit rate tracking Mar 12, 2024
mix.lock Show resolved Hide resolved
:ets.new(__MODULE__, [
:public,
:named_table,
:ordered_set,
Copy link
Contributor

@ruslandoga ruslandoga Mar 13, 2024

Choose a reason for hiding this comment

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

I wonder why not just :set? It doesn't seem to perform any range queries.

Copy link
Contributor

@ruslandoga ruslandoga Mar 13, 2024

Choose a reason for hiding this comment

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

I ran a benchmark and there doesn't seem to be any significant difference in ips

Benchee.run(%{
  "hit" => fn ->
    Enum.each(1..10, fn i ->
      Plausible.Cache.Stats.record_hit(i)
    end)
  end,
  "miss" => fn ->
    Enum.each(1..10, fn i ->
      Plausible.Cache.Stats.record_miss(i)
    end)
  end
})
ordered set
Operating System: macOS
CPU Information: Apple M2
Number of Available Cores: 8
Available memory: 8 GB
Elixir 1.16.0
Erlang 26.2.1
JIT enabled: true

Benchmark suite executing with the following configuration:
warmup: 2 s
time: 5 s
memory time: 0 ns
reduction time: 0 ns
parallel: 1
inputs: none specified
Estimated total run time: 14 s

Benchmarking hit ...
Benchmarking miss ...
Calculating statistics...
Formatting results...

Name           ips        average  deviation         median         99th %
hit       551.83 K        1.81 μs   ±697.99%        1.67 μs        2.33 μs
miss      507.10 K        1.97 μs   ±777.85%        1.79 μs        2.54 μs

Comparison: 
hit       551.83 K
miss      507.10 K - 1.09x slower +0.160 μs
set
Operating System: macOS
CPU Information: Apple M2
Number of Available Cores: 8
Available memory: 8 GB
Elixir 1.16.0
Erlang 26.2.1
JIT enabled: true

Benchmark suite executing with the following configuration:
warmup: 2 s
time: 5 s
memory time: 0 ns
reduction time: 0 ns
parallel: 1
inputs: none specified
Estimated total run time: 14 s

Benchmarking hit ...
Benchmarking miss ...
Calculating statistics...
Formatting results...

Name           ips        average  deviation         median         99th %
miss      611.56 K        1.64 μs   ±750.33%        1.50 μs        2.08 μs
hit       601.67 K        1.66 μs   ±721.77%        1.50 μs        2.17 μs

Comparison: 
miss      611.56 K
hit       601.67 K - 1.02x slower +0.0269 μs

I thought :set would be faster...

Copy link
Contributor

@ruslandoga ruslandoga Mar 13, 2024

Choose a reason for hiding this comment

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

Seems like the telemetry.execute "dominates" over the ets ops hence the little difference. Either way it's super fast.

record_hit vs bump bench
Benchee.run(
  %{
    "hit" => fn ->
      Enum.each(1..10, fn i ->
        Plausible.Cache.Stats.record_hit(i)
      end)
    end,
    "bump" => fn ->
      Enum.each(1..10, fn i ->
        Plausible.Cache.Stats.bump(i, :hit)
      end)
    end
  },
  profile_after: true
)
Operating System: macOS
CPU Information: Apple M2
Number of Available Cores: 8
Available memory: 8 GB
Elixir 1.16.0
Erlang 26.2.1
JIT enabled: true

Benchmark suite executing with the following configuration:
warmup: 2 s
time: 5 s
memory time: 0 ns
reduction time: 0 ns
parallel: 1
inputs: none specified
Estimated total run time: 14 s

Benchmarking bump ...
Benchmarking hit ...
Calculating statistics...
Formatting results...

Name           ips        average  deviation         median         99th %
bump        1.58 M        0.63 μs  ±3187.16%        0.58 μs        0.79 μs
hit         0.62 M        1.61 μs   ±751.28%        1.46 μs        2.08 μs

Comparison: 
bump        1.58 M
hit         0.62 M - 2.53x slower +0.97 μs

Profiling bump with eprof...

Profile results of #PID<0.910.0>
#                                               CALLS     % TIME µS/CALL
Total                                              49 100.0   14    0.29
anonymous fn/3 in Enum.each/2                      10  0.00    0    0.00
Enum.reduce_range/5                                 6  0.00    0    0.00
Enum.each/2                                         1  0.00    0    0.00
anonymous fn/1 in :elixir_compiler_2.__FILE__/1    10  0.00    0    0.00
anonymous fn/0 in :elixir_compiler_2.__FILE__/1     1  0.00    0    0.00
:erlang.apply/2                                     1  7.14    1    1.00
Plausible.Cache.Stats.bump/2                       10  7.14    1    0.10
:ets.update_counter/4                              10 85.71   12    1.20

Profile done over 8 matching functions

Profiling hit with eprof...

Profile results of #PID<0.912.0>
#                                               CALLS     % TIME µS/CALL
Total                                             139 100.0   17    0.12
anonymous fn/3 in Enum.each/2                      10  0.00    0    0.00
Enum.reduce_range/5                                 6  0.00    0    0.00
Enum.each/2                                         1  0.00    0    0.00
Plausible.Cache.Stats.record_hit/1                 10  0.00    0    0.00
anonymous fn/1 in :elixir_compiler_2.__FILE__/1    10  0.00    0    0.00
anonymous fn/0 in :elixir_compiler_2.__FILE__/1     1  0.00    0    0.00
:lists.foreach/2                                   10  0.00    0    0.00
:erlang.apply/2                                     1  5.88    1    1.00
:telemetry_handler_table.list_for_event/1          10  5.88    1    0.10
:telemetry.execute/3                               10  5.88    1    0.10
Plausible.Cache.Stats.bump/2                       10  5.88    1    0.10
:lists.foreach_1/2                                 20  5.88    1    0.05
anonymous fn/4 in :telemetry.execute/3             10 11.76    2    0.20
:ets.update_counter/4                              10 17.65    3    0.30
:ets.lookup/2                                      10 17.65    3    0.30
Plausible.Cache.Stats.handle_telemetry_event/4     10 23.53    4    0.40

Profile done over 16 matching functions

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh cool, thanks for running these.

Re:

I thought :set would be faster...

From your benchmarks it looks like it is, still? Or do you mean you expected a bigger difference? I guess it doesn't cost anything to change it to :set, do you agree?

@zoldar even suggested atomic counters, but I thought with the necessity of keeping refs, it wasn't worth it. WYT?

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Contributor

@ruslandoga ruslandoga Mar 13, 2024

Choose a reason for hiding this comment

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

Or do you mean you expected a bigger difference?

Yes, I expected the difference to be more noticeable.

I guess it doesn't cost anything to change it to :set, do you agree?

I agree :)

atomic counters WYT?

In my tests for the rate limiter they were somehow slower than ets.update_counter but the use case there was slightly different.

Later I learned there were two types of counters, atomics and something else created with erts_internal:counters_new. The latter are supposed to be even faster. They are used in code coverage. But I didn't update the benchmark since I liked how easy ets.update_counter was.

I think Stats are already fast enough :)

mix.exs Outdated Show resolved Hide resolved
@aerosol aerosol merged commit 59afa20 into master Mar 14, 2024
5 checks passed
@aerosol aerosol deleted the reapply-cache-change branch March 14, 2024 07:06
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