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

Use erlang:whereis/1 for checking if a store is running #296

Merged
merged 1 commit into from
Sep 12, 2024

Conversation

the-mikedavis
Copy link
Member

A store's process uses its StoreId as its registered name. This is a public interface of Ra so we can depend on it. Reading from key metrics counters is already very fast but switching to whereis/1 eliminates basically all overhead of this function. When used heavily (for example in RabbitMQ while publishing and consuming rapidly) the CPU time spent on is_store_running/1 disappears from the output of a perf recording and a flamegraph.

This is a follow-up to #292 based on the post-merge discussion.

@the-mikedavis the-mikedavis added the enhancement New feature or request label Sep 11, 2024
@the-mikedavis the-mikedavis self-assigned this Sep 11, 2024
Copy link

codecov bot commented Sep 11, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.65%. Comparing base (2d04459) to head (ef07168).
Report is 2 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #296      +/-   ##
==========================================
+ Coverage   89.54%   89.65%   +0.11%     
==========================================
  Files          21       21              
  Lines        3195     3191       -4     
==========================================
  Hits         2861     2861              
+ Misses        334      330       -4     
Flag Coverage Δ
erlang-25 88.90% <100.00%> (+0.20%) ⬆️
erlang-26 89.28% <100.00%> (+0.04%) ⬆️
erlang-27 89.28% <100.00%> (-0.02%) ⬇️
os-ubuntu-latest 89.62% <100.00%> (+0.26%) ⬆️
os-windows-latest 89.56% <100.00%> (+0.11%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@dumbbell dumbbell left a comment

Choose a reason for hiding this comment

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

I still consider this is knowledge of an implementation detail of Ra.

I know that the ra_server_id() type is described as a registered process name in src/ra.hrl. However, it is explicitly documented as an internal thing:

  • The Ra server ID concept is described in docs/internals/INTERNALS.md.
  • When the server_id() type is defined and exported, it is also marked as internal:
    %% export some internal types
    ...
    -type server_id() :: ra_server_id().
    ...

Therefore, I’m ok to accept the change, but I would like that a comment is added to clarify this. For instance:

    %% FIXME: Ra has no API to know if a Ra server is running or not. We could
    %% use a public API such as `ra:key_metrics/2', but unfortunalety, it is
    %% not as efficient as querying the process directly. Therefore, we bypass
    %% Ra and rely on the fact that the server ID is internally a registered
    %% process name and resolve it to determine if it is running.
    Runs = erlang:whereis(StoreId) =/= undefined,

A store's process uses its `StoreId` as its registered name. This is a
public interface of Ra so we can depend on it. Reading from key metrics
counters is already very fast but switching to `whereis/1` eliminates
basically all overhead of this function. When used heavily (for example
in RabbitMQ while publishing and consuming rapidly) the CPU time spent
on `is_store_running/1` disappears from the output of a perf recording
and a flamegraph.

Co-authored-by: Jean-Sébastien Pédron <jean-sebastien.pedron@dumbbell.fr>
Copy link
Member

@dumbbell dumbbell left a comment

Choose a reason for hiding this comment

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

Thanks!

@dumbbell dumbbell merged commit 15e7119 into main Sep 12, 2024
12 checks passed
@dumbbell dumbbell deleted the md/is_store_running-whereis branch September 12, 2024 13:32
@michaelklishin michaelklishin added this to the v0.16.0 milestone Sep 12, 2024
@michaelklishin
Copy link
Member

We don't have a 0.15.1 milestone (maybe we should) so I have set it to 0.16.0 because it's better than not having any details.

@dumbbell
Copy link
Member

Thank you, I forgot to set the milestone…

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants