Skip to content

[Metrics SDK] Use nostd::function_ref in AttributesHashMap #3393

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
May 8, 2025
Merged

[Metrics SDK] Use nostd::function_ref in AttributesHashMap #3393

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9ac554a
[Metrics SDK] Use nostd::function_ref in AttributesHashMap for aggreg…
yashykt May 7, 2025
eb75c76
Update changelog
yashykt May 7, 2025
54b0785
try to fix iwyu
yashykt May 8, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,9 @@ Increment the:

## [Unreleased]

* [Metrics SDK] Use nostd::function_ref in AttributesHashMap
[#3393](https://github.com/open-telemetry/opentelemetry-cpp/pull/3393)

* [SDK] Base2 exponential histogram aggregation
[#3175](https://github.com/open-telemetry/opentelemetry-cpp/pull/3346)

Expand Down
19 changes: 11 additions & 8 deletions sdk/include/opentelemetry/sdk/metrics/state/attributes_hashmap.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,9 +74,10 @@ class AttributesHashMapWithCustomHash
* If not present, it uses the provided callback to generate
* value and store in the hash
*/
Aggregation *GetOrSetDefault(const opentelemetry::common::KeyValueIterable &attributes,
const AttributesProcessor *attributes_processor,
std::function<std::unique_ptr<Aggregation>()> aggregation_callback)
Aggregation *GetOrSetDefault(
const opentelemetry::common::KeyValueIterable &attributes,
const AttributesProcessor *attributes_processor,
nostd::function_ref<std::unique_ptr<Aggregation>()> aggregation_callback)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it necessary to use nostd::function_ref instead of std::function?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member

@lalitb lalitb May 8, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks @yashykt , @psx95 for benchmark.

@yashykt Can you add this reasoning in the PR desc. The changes look good once CI is fixed.

Separately. we may want to revisit other usage of std::function in the SDK if they are candidate for replacing with nostd::function_ref.

{
// TODO: avoid constructing MetricAttributes from KeyValueIterable for
// hash_map_.find which is a heavy operation
Expand All @@ -97,8 +98,9 @@ class AttributesHashMapWithCustomHash
return result.first->second.get();
}

Aggregation *GetOrSetDefault(const MetricAttributes &attributes,
std::function<std::unique_ptr<Aggregation>()> aggregation_callback)
Aggregation *GetOrSetDefault(
const MetricAttributes &attributes,
nostd::function_ref<std::unique_ptr<Aggregation>()> aggregation_callback)
{
auto it = hash_map_.find(attributes);
if (it != hash_map_.end())
Expand All @@ -115,8 +117,9 @@ class AttributesHashMapWithCustomHash
return hash_map_[attributes].get();
}

Aggregation *GetOrSetDefault(MetricAttributes &&attributes,
std::function<std::unique_ptr<Aggregation>()> aggregation_callback)
Aggregation *GetOrSetDefault(
MetricAttributes &&attributes,
nostd::function_ref<std::unique_ptr<Aggregation>()> aggregation_callback)
{
auto it = hash_map_.find(attributes);
if (it != hash_map_.end())
Expand Down Expand Up @@ -207,7 +210,7 @@ class AttributesHashMapWithCustomHash
size_t attributes_limit_;

Aggregation *GetOrSetOveflowAttributes(
std::function<std::unique_ptr<Aggregation>()> aggregation_callback)
nostd::function_ref<std::unique_ptr<Aggregation>()> aggregation_callback)
{
auto agg = aggregation_callback();
return GetOrSetOveflowAttributes(std::move(agg));
Expand Down
Loading