[Metrics SDK] Use nostd::function_ref in AttributesHashMap#3393
[Metrics SDK] Use nostd::function_ref in AttributesHashMap#3393ThomsonTan merged 3 commits intoopen-telemetry:mainfrom
Conversation
✅ Deploy Preview for opentelemetry-cpp-api-docs canceled.
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3393 +/- ##
==========================================
+ Coverage 89.96% 89.98% +0.02%
==========================================
Files 211 211
Lines 6812 6812
==========================================
+ Hits 6128 6129 +1
+ Misses 684 683 -1
🚀 New features to boost your workflow:
|
| Aggregation *GetOrSetDefault( | ||
| const opentelemetry::common::KeyValueIterable &attributes, | ||
| const AttributesProcessor *attributes_processor, | ||
| nostd::function_ref<std::unique_ptr<Aggregation>()> aggregation_callback) |
There was a problem hiding this comment.
Is it necessary to use nostd::function_ref instead of std::function?
There was a problem hiding this comment.
I've got a benchmark at https://github.com/grpc/grpc/blob/507a5de9483c9b8675e626977a51d9286f4104f0/test/cpp/microbenchmarks/bm_stats_plugin.cc#L81. After this change, it goes down from 200+ns to ~20ns.
Looking at the implementation, we don't need to make a copy of the passed in callable, so it seems desirable to do this.
There was a problem hiding this comment.
I compared the BM_AttributseHashMap benchmark result for this PR vs the PR from the last commit on main,
there does seem to be reduction in the realtime and cpu time:
Current HEAD (merged PR#3383):
"benchmarks": [
{
"name": "BM_AttributseHashMap",
"family_index": 0,
"per_family_instance_index": 0,
"run_name": "BM_AttributseHashMap",
"run_type": "iteration",
"repetitions": 1,
"repetition_index": 0,
"threads": 1,
"iterations": 7,
"real_time": 3.8004398345947266e+07,
"cpu_time": 2.0847316142857142e+07,
"time_unit": "ns"
}
]This PR's CI:
"benchmarks": [
{
"name": "BM_AttributseHashMap",
"family_index": 0,
"per_family_instance_index": 0,
"run_name": "BM_AttributseHashMap",
"run_type": "iteration",
"repetitions": 1,
"repetition_index": 0,
"threads": 1,
"iterations": 8,
"real_time": 2.9205620288848877e+07,
"cpu_time": 2.0728138000000000e+07,
"time_unit": "ns"
}
]Additionally, I think we are using the function_ref to immediately call and compute the result here, which makes me think it might be ok to add this change.
Note: Result located in sdk/test/metrics/attributes_hashmap_benchmark_result.json
There was a problem hiding this comment.
psx95
left a comment
psx95
left a comment
There was a problem hiding this comment.
The iwyu CI check is failing which needs to be addressed.
[Metrics SDK] Use nostd::function_ref in AttributesHashMap (open-telemetry#3393)
4 participants
Changes
Use nostd::function_ref in AttributesHashMap for
aggregation_callback.A benchmark at https://github.com/grpc/grpc/blob/507a5de9483c9b8675e626977a51d9286f4104f0/test/cpp/microbenchmarks/bm_stats_plugin.cc#L81. shows time go down from 200+ns to ~20ns.
Additionally, with the benchmarks run by @psx95 on BM_AttributseHashMap -
Current HEAD (merged PR#3383):
This PR's CI:
Looking at the implementation, we don't need to make a copy of the passed in callable, so it seems desirable to do this.
For significant contributions please make sure you have completed the following items:
CHANGELOG.mdupdated for non-trivial changes