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

Hashed utility for ValueMap to improve hashing performance #2296

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

Conversation

fraillt
Copy link
Contributor

@fraillt fraillt commented Nov 11, 2024

Changes

Remove redundant hashing in ValueMap, by using new Hashed type. It's similar to AttributeSet, but is generic, and support owned and non-owned types (similar to Cow type).

On happy path, there's very low performance increase, but Counter_Overflow has improved significantly.

The whole reason for this PR, is not 2% percent performance increase, but being able to reuse same hashed results when implementing sharding #2297.

cargo bench --bench metrics_counter
Counter_Created
Counter_Add_Sorted      time:   [121.68 ns 121.86 ns 122.08 ns]
                        change: [-3.1490% -2.7896% -2.4834%] (p = 0.00 < 0.05)
                        Performance has improved.
Found 7 outliers among 100 measurements (7.00%)
  1 (1.00%) low severe
  1 (1.00%) low mild
  3 (3.00%) high mild
  2 (2.00%) high severe

Counter_Created
Counter_Add_Unsorted    time:   [125.76 ns 126.25 ns 126.88 ns]
                        change: [-5.3282% -4.3370% -3.5178%] (p = 0.00 < 0.05)
                        Performance has improved.
Found 10 outliers among 100 measurements (10.00%)
  4 (4.00%) high mild
  6 (6.00%) high severe

Counter_Created
Counter_Overflow        time:   [420.32 ns 420.48 ns 420.64 ns]
                        change: [-36.270% -36.208% -36.146%] (p = 0.00 < 0.05)
                        Performance has improved.
Found 4 outliers among 100 measurements (4.00%)
  2 (2.00%) high mild
  2 (2.00%) high severe

Benchmarking ThreadLocal_Random_Generator_5: Collecting 100 samples in estimated 5.0001 s (152M ThreadLocal_Random_Generator_5
                        time:   [32.440 ns 32.468 ns 32.521 ns]
                        change: [-0.0468% +0.0284% +0.1066%] (p = 0.51 > 0.05)
                        No change in performance detected.
Found 5 outliers among 100 measurements (5.00%)
  1 (1.00%) low mild
  2 (2.00%) high mild
  2 (2.00%) high severe

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)

@fraillt fraillt requested a review from a team as a code owner November 11, 2024 20:41
Copy link

codecov bot commented Nov 11, 2024

Codecov Report

Attention: Patch coverage is 93.28358% with 9 lines in your changes missing coverage. Please review.

Project coverage is 79.4%. Comparing base (0cc2cd5) to head (2aa01ee).
Report is 5 commits behind head on main.

Files with missing lines Patch % Lines
opentelemetry-sdk/src/metrics/internal/hashed.rs 87.5% 9 Missing ⚠️
Additional details and impacted files
@@          Coverage Diff           @@
##            main   #2296    +/-   ##
======================================
  Coverage   79.4%   79.4%            
======================================
  Files        122     123     +1     
  Lines      20783   20905   +122     
======================================
+ Hits       16504   16619   +115     
- Misses      4279    4286     +7     

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

@@ -29,6 +29,7 @@ tokio = { workspace = true, features = ["rt", "time"], optional = true }
tokio-stream = { workspace = true, optional = true }
http = { workspace = true, optional = true }
tracing = {workspace = true, optional = true}
rustc-hash = "2.0.0"
Copy link
Contributor Author

@fraillt fraillt Nov 11, 2024

Choose a reason for hiding this comment

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

I should probably try other hash functions, but results that I posted is with this crate.
Generally, my idea was, that if it is good enough for rustc compiler (which operates on a lot of small strings), it should be perfect fit for attribute sets too.
But if you want, I might as well test with other crates.

I remember that @cijothomas pointed me to this comment:
https://github.com/open-telemetry/opentelemetry-rust/pull/1564/files#diff-efe82800c0ca89e5cdf4c14f0674f753b0148351f0bfad79f89b20d295c4e6e4R58
but I'm a bit afraid that attribute-sets in current benchmarks/stress-test is different from real world usage (using opentelemetry-semantic-conventions), so these tests might not be vary accurate as well...

Copy link
Contributor

@utpilla utpilla Nov 13, 2024

Choose a reason for hiding this comment

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

In general, when running benchmarks to compare performance for metric updates, please run the benchmarks in metrics_counter.rs instead of metric.rs. We haven't really fine-tuned the benchmarks in metrics.rs in a while. We might also remove if it doesn't help that much.

Copy link
Contributor

@utpilla utpilla left a comment

Choose a reason for hiding this comment

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

@fraillt This PR is trying to bring two changes at once.

Could you please split this into two different PRs?

  1. One PR which only has changes related to caching the computed hash value
  2. Another one which uses a different Hasher (could you try aHash?). Also, please note this would have to be feature flagged. We do not want to use a custom hasher by default.

@fraillt fraillt force-pushed the value-map-hash-once branch from 2aa01ee to 8e9d216 Compare November 13, 2024 17:06
@fraillt
Copy link
Contributor Author

fraillt commented Nov 13, 2024

@fraillt This PR is trying to bring two changes at once.

Could you please split this into two different PRs?

1. One PR which only has changes related to caching the computed hash value

2. Another one which uses a different Hasher (could you try [aHash](https://github.com/tkaitchuck/aHash)?). Also, please note this would have to be feature flagged. We do not want to use a custom hasher by default.

Updated code and PR description.
I'll create another PR, with different hasher functions once/if this lands.

@cijothomas
Copy link
Member

cijothomas commented Nov 14, 2024

Ran stress/benchmarks
Benchmarks - within noise.(Overflow scenario does improve due to avoiding the need for hash calc multiple times)
Stress test metrics_counter - Drops from 14.5M/sec to 11M/sec

@cijothomas
Copy link
Member

cijothomas commented Nov 14, 2024

I see this will have huge value if we were to implement sharding ourselves, so caching the hash value will definitely help. Before proceeding further, I'd like to suggest to confirm that implementing sharding ourselves is the right direction. See #2288 (comment)

@fraillt
Copy link
Contributor Author

fraillt commented Nov 14, 2024

Benchmarks - within noise.(Overflow scenario does improve due to avoiding the need for hash calc multiple times)
Stress test metrics_counter - Drops from 14.5M/sec to 11M/sec

Yes, benchmarks are basically within the noise, but it's technically an improvement, this revision avoids extra hashing in HashMap, because of HashedNoOpBuilder on hashmap.

Regarding stress they are so weird and sensitive on so many factors... on my current machine, I consistently observe opposite results, maybe ~10% improvement, on 1.81 rust version. but your machine is totally different, -20% performance degradation... wow :)
I have my old SBC (single-board-computer) with ARM processor, maybe I'll need to wipe off the dust, and use it for some stress testing :)

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