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 additional ThreadStatic storage to avoid allocation while sorting… #2865

Merged

Conversation

utpilla
Copy link
Contributor

@utpilla utpilla commented Feb 5, 2022

Fixes #2838

Changes

  • Add additional ThreadStatic storage to avoid allocation while sorting tags

Benchmark Results:

The benchmarks numbers are comparable to main branch. This is expected since the benchmark methods do not change the order of the tag Keys.

// * Summary *

BenchmarkDotNet=v0.13.1, OS=Windows 10.0.22000
Intel Core i7-9700 CPU 3.00GHz, 1 CPU, 8 logical and 8 physical cores
.NET SDK=6.0.101
[Host] : .NET 6.0.1 (6.0.121.56705), X64 RyuJIT
DefaultJob : .NET 6.0.1 (6.0.121.56705), X64 RyuJIT

main branch

Method AggregationTemporality Mean Error StdDev Allocated
CounterHotPath Cumulative 15.59 ns 0.058 ns 0.054 ns -
CounterWith1LabelsHotPath Cumulative 55.35 ns 0.189 ns 0.158 ns -
CounterWith3LabelsHotPath Cumulative 140.92 ns 0.855 ns 0.758 ns -
CounterWith5LabelsHotPath Cumulative 227.50 ns 1.032 ns 0.965 ns -
CounterWith6LabelsHotPath Cumulative 256.62 ns 1.901 ns 1.778 ns -
CounterWith7LabelsHotPath Cumulative 292.72 ns 2.457 ns 2.298 ns -
CounterHotPath Delta 15.59 ns 0.099 ns 0.093 ns -
CounterWith1LabelsHotPath Delta 56.54 ns 0.152 ns 0.134 ns -
CounterWith3LabelsHotPath Delta 141.70 ns 0.762 ns 0.713 ns -
CounterWith5LabelsHotPath Delta 223.16 ns 0.755 ns 0.630 ns -
CounterWith6LabelsHotPath Delta 253.95 ns 1.706 ns 1.425 ns -
CounterWith7LabelsHotPath Delta 282.17 ns 0.924 ns 0.819 ns -

With the new changes

Method AggregationTemporality Mean Error StdDev Allocated
CounterHotPath Cumulative 15.58 ns 0.058 ns 0.054 ns -
CounterWith1LabelsHotPath Cumulative 56.38 ns 0.259 ns 0.243 ns -
CounterWith3LabelsHotPath Cumulative 138.30 ns 0.584 ns 0.546 ns -
CounterWith5LabelsHotPath Cumulative 225.52 ns 2.162 ns 2.023 ns -
CounterWith6LabelsHotPath Cumulative 256.01 ns 3.152 ns 2.949 ns -
CounterWith7LabelsHotPath Cumulative 287.17 ns 1.709 ns 1.515 ns -
CounterHotPath Delta 15.66 ns 0.120 ns 0.112 ns -
CounterWith1LabelsHotPath Delta 59.25 ns 0.363 ns 0.340 ns -
CounterWith3LabelsHotPath Delta 137.90 ns 0.610 ns 0.476 ns -
CounterWith5LabelsHotPath Delta 224.39 ns 1.401 ns 1.310 ns -
CounterWith6LabelsHotPath Delta 253.78 ns 1.123 ns 0.995 ns -
CounterWith7LabelsHotPath Delta 285.55 ns 1.654 ns 1.467 ns -

@utpilla utpilla requested a review from a team February 5, 2022 02:41
@codecov
Copy link

codecov bot commented Feb 5, 2022

Codecov Report

Merging #2865 (b82f7b6) into main (edede3a) will increase coverage by 0.00%.
The diff coverage is 92.59%.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #2865   +/-   ##
=======================================
  Coverage   85.16%   85.16%           
=======================================
  Files         259      259           
  Lines        9330     9345   +15     
=======================================
+ Hits         7946     7959   +13     
- Misses       1384     1386    +2     
Impacted Files Coverage Δ
src/OpenTelemetry/Metrics/ThreadStaticStorage.cs 85.45% <88.88%> (-0.60%) ⬇️
src/OpenTelemetry/Metrics/AggregatorStore.cs 83.05% <100.00%> (+0.29%) ⬆️
...metryProtocol/Implementation/ActivityExtensions.cs 91.44% <0.00%> (-0.91%) ⬇️
...tation/OpenTelemetryProtocolExporterEventSource.cs 85.00% <0.00%> (+10.00%) ⬆️

@github-actions
Copy link
Contributor

This PR was marked stale due to lack of activity and will be closed in 7 days. Commenting or Pushing will instruct the bot to automatically remove the label. This bot runs once per day.

@github-actions github-actions bot added the Stale Issues and pull requests which have been flagged for closing due to inactivity label Feb 12, 2022
Copy link
Member

@alanwest alanwest left a comment

Choose a reason for hiding this comment

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

LGTM

@alanwest alanwest removed the Stale Issues and pull requests which have been flagged for closing due to inactivity label Feb 16, 2022
@github-actions
Copy link
Contributor

This PR was marked stale due to lack of activity and will be closed in 7 days. Commenting or Pushing will instruct the bot to automatically remove the label. This bot runs once per day.

@github-actions github-actions bot added the Stale Issues and pull requests which have been flagged for closing due to inactivity label Mar 29, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Apr 6, 2022

Closed as inactive. Feel free to reopen if this PR is still being worked on.

@github-actions github-actions bot closed this Apr 6, 2022
@utpilla utpilla reopened this Apr 7, 2022
@cijothomas cijothomas merged commit 1d16e0b into open-telemetry:main Apr 15, 2022
@utpilla utpilla deleted the utpilla/Update-Metric-AggregatorStore branch November 23, 2023 03:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Stale Issues and pull requests which have been flagged for closing due to inactivity
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Avoid allocation in AggregatorStore for MetricPoint lookup
4 participants