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

Add benchmark for user_events enabled check #133

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

Conversation

lalitb
Copy link
Member

@lalitb lalitb commented Nov 27, 2024

Changes

Initial benchmark to check if the tracepoint is enabled. Need to add more benchmark to write to the tracepoint.

Merge requirement checklist

  • CONTRIBUTING guidelines followed
  • Unit tests added/updated (if applicable)
  • Appropriate CHANGELOG.md files updated for non-trivial, user-facing changes
  • Changes in public API reviewed (if applicable)

@lalitb lalitb requested a review from a team as a code owner November 27, 2024 06:46
Copy link

codecov bot commented Nov 27, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 54.1%. Comparing base (67b30de) to head (ac193e0).

Additional details and impacted files
@@          Coverage Diff          @@
##            main    #133   +/-   ##
=====================================
  Coverage   54.1%   54.1%           
=====================================
  Files         42      42           
  Lines       6276    6276           
=====================================
  Hits        3401    3401           
  Misses      2875    2875           

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

@lalitb
Copy link
Member Author

lalitb commented Dec 10, 2024

Have added the single and concurrent benchmark test to check if the trace-point is enabled, The result shows the regression as the number of threads are increased:

// provider_find_set_single time:   [3.9862 ns 3.9898 ns 3.9937 ns]
// provider_find_set_concurrent_0threads time:   [3.9937 ns 3.9978 ns 4.0024 ns]
// provider_find_set_concurrent_2threads:  time:    time:   [111.52 ns 114.61 ns 117.38 ns]
// provider_find_set_concurrent_4threads: time:   time:   [199.18 ns 203.92 ns 208.43 ns]
// provider_find_set_concurrent_8threads time:   time:   [199.18 ns 203.92 ns 208.43 ns]

Will validate the test setup once again, and then we need to check tracepoint sdk for contention.

}

/// Benchmark `find_set` with a parameterized number of concurrent threads
fn benchmark_find_set_concurrent(c: &mut Criterion) {
Copy link
Member

Choose a reason for hiding this comment

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

maybe we can simply re-use the existing stress test? (copy it over to contribute repo?)

@lalitb
Copy link
Member Author

lalitb commented Dec 11, 2024

// provider_find_set_single time: [3.9862 ns 3.9898 ns 3.9937 ns]
// provider_find_set_concurrent_0threads time: [3.9937 ns 3.9978 ns 4.0024 ns]
// provider_find_set_concurrent_2threads: time: time: [111.52 ns 114.61 ns 117.38 ns]
// provider_find_set_concurrent_4threads: time: time: [199.18 ns 203.92 ns 208.43 ns]
// provider_find_set_concurrent_8threads time: time: [199.18 ns 203.92 ns 208.43 ns]

Added stress test for this in #138. These are the results from stress test are as below (courtesy - chatgpt to generate it from console output)

Threads Throughput_1 (Millions) Throughput_2 (Millions) Average_Throughput (Millions)
1 232.87 233.06 232.97
2 23.90 27.23 25.57
3 21.06 21.69 21.38
4 20.87 21.37 21.12
5 20.78 21.89 21.33
7 21.47 21.30 21.38
8 22.15 21.87 22.01
10 23.77 23.17 23.47
12 24.60 19.72 22.16
15 25.72 25.56 25.64
16 26.72 26.73 26.72
20 26.35 26.34 26.35

At least. in both benchmark and stress test, there is substantial difference between single threaded, and multi-threaded results. The performance seems to be degraded substantially as we go from single thread to 2 threads:
benchmark latency: 3.99 ns to 111.52ns
stress throughput: 233 millions/sec to 23.90 millons/sec.

The flamegraph doesn't show much. Seems the find_set and enabled methods are aggressively inline'd, and the result doesn't go beyond the find_set call.

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