[POC] Baggage API : return `Baggage` instead of `shared_ptr<Baggage>` using PImpl idiom #836

lalitb · 2021-06-08T15:10:40Z

Not to merge for now - this is proof of concept for Baggage Implementation using Pimpl Idiom, as part of discussion #828

For significant contributions please make sure you have completed the following items:

CHANGELOG.md updated for non-trivial changes
Unit tests have been added
Changes in public API reviewed

codecov · 2021-06-08T15:35:00Z

Codecov Report

Merging #836 (c6aaf37) into main (b073e8b) will increase coverage by 0.03%.
The diff coverage is 97.68%.

@@            Coverage Diff             @@
##             main     #836      +/-   ##
==========================================
+ Coverage   95.50%   95.53%   +0.03%     
==========================================
  Files         156      157       +1     
  Lines        6620     6632      +12     
==========================================
+ Hits         6322     6335      +13     
+ Misses        298      297       -1

Impacted Files	Coverage Δ
...telemetry/baggage/propagation/baggage_propagator.h	`92.86% <83.34%> (+6.20%)`	⬆️
api/include/opentelemetry/baggage/baggage_impl.h	`97.30% <97.30%> (ø)`
api/include/opentelemetry/baggage/baggage.h	`100.00% <100.00%> (+2.71%)`	⬆️
api/test/baggage/baggage_test.cc	`100.00% <100.00%> (ø)`
...est/baggage/propagation/baggage_propagator_test.cc	`100.00% <100.00%> (ø)`

jsuereth · 2021-06-08T15:44:10Z

Nice work!

One question:

Did we have performance benchmarks for this like we do for span processors? If not we should open a bug going forward to check performance of baggage propagation.

lalitb · 2021-06-08T15:58:55Z

Did we have performance benchmarks for this like we do for span processors? If not we should open a bug going forward to check performance of baggage propagation.

Good point. Will raise a bug for this.

maxgolov · 2021-06-08T19:59:58Z

api/include/opentelemetry/baggage/baggage.h

-
-    return ret;
-  }
+  std::string ToHeader() const { return baggage_impl_->ToHeader(); }


Since this is in API, is this std::string type ever going to be exposed across DLL / Shared Library boundary? Even if it's a private method, isn't this still something where we'd rather return a nostd::string_view instead of std::string ?

It's a non-virtual function, so it should be safe to return std::string, it won't break ABI stability. Though we can change it as below had it been a virtual function:

void ToHeader(nostd::string &header) { }

api/include/opentelemetry/baggage/baggage_impl.h

pyohannes · 2021-06-09T00:34:32Z

api/include/opentelemetry/baggage/baggage.h

-  // Store entries in a C-style array to avoid using std::array or std::vector.
-  nostd::unique_ptr<opentelemetry::common::KeyValueProperties> kv_properties_;
+  Baggage(BaggageImpl *baggage_impl) : baggage_impl_(baggage_impl) {}
+  nostd::shared_ptr<BaggageImpl> baggage_impl_;


Maybe I'm missing something: the shared_ptr is wrapped now and not exposed, but isn't it the goal to provide a way to get completely rid of shared_ptr usage? How would that work with this solution?

Yes, getting rid of shared_ptr, and dynamic allocation is the ideal solution, but that won't be easy to implement as the same baggage can also be stored in Context.
The current approach removes the control of smart pointer management away from external users. It is inspired from:
opencensus : https://github.com/census-instrumentation/opencensus-cpp/blob/9461de16eef2286a4a062e890548ab148aa94c55/opencensus/trace/span.h#L194
and
https://vector-of-bool.github.io/2018/12/02/smart-pointer-apis.html

Do you see any better approach to handle it? I saw your comment on returning unique_ptr and convert to shared_ptr later. How can that be done, as these would be two different pointers. The unique_ptr would get invalidated once it is moved to shared_ptr, and the user would still be trying to use unique_ptr.

So, in essence, we are not really making it better-perf for any of the default exporters.. we making it worse by creating yet another abstraction layer with a shared_ptr. Okay. 🤔

I'd like to see a Google Benchmark run, for a synthetic loop with some work in it, before and after. This issue would be a great start to do the initial set of measurements #837

Yes true. In fact, it adds another layer of redirection, along with a small cost incurred for storing <shared_ptr> as a data member. This should be a solution only if we plan not to use smart pointers across API. It's more of a design decision than a performance solution.

Let's run some numbers. I am also interested in synchronous, non-batching, streaming exporter. And in good numbers.

So, in essence, we are not really making it better-perf for any of the default exporters.. we making it worse by creating yet another abstraction layer with a shared_ptr. Okay.

I kind of agree, I don't see how this improves things. We still have a baked-in shared pointer which we hide from the user, and the user has no way to change that.

I don't see any benefits of this refactor. If it's a purely stylistic effort, I have no strong opinion about that.

Do you see any better approach to handle it?

I see two possible ways:

Implementing a purely stack-based solution ourselves. This would require a re-implementation of the underlying key/value data structure.

Giving the user means so they can provide their own baggage implementation, which follows their own memory management strategy.

Maybe there are other ways not coming to my mind. If we go with (2), the same approach could also be used to avoid using shared pointers for spans.

bogdandrutu

Nice work, thanks.

bogdandrutu · 2021-06-09T05:53:48Z

api/include/opentelemetry/baggage/baggage_impl.h

+static constexpr char kMembersSeparator   = ',';
+static constexpr char kMetadataSeparator  = ';';
+
+class BaggageImpl


Can this be hidden? Maybe have an "internal" as part of the namespace and we add a rule that everything in the internal package is private? "baggage::internal::BaggageImpl"?

maxgolov · 2021-06-15T20:59:18Z

I think perf numbers presently show that this refactor isn't making anything better (it is x1.7 times worse perf for a barebone span)... Also it takes away the partial ownership semantics.

I think it'd be best if we carefully allow additional API, maybe in a separate namespace? To address the ask for a value object return type without refactoring / breaking of what we already got working in v1.0. Especially given that refactor is not good:

degrades performance.
takes away quite important feature that may be relevant to multi-threaded apps, with context propagation to worker threads in a pool. shared_ptr allows you to pass the Span to child threads / workers, whereas unique_ptr won't allow to do that safely.

Can you elaborate on those two points, if maybe I'm misunderstanding / misinterpreting something.. Can we take another stab at this after GA? Options to consider:

assignment operator on SpanContext that allows to extract the object from shared_ptr<Span>.
separate namespace with value objects.
maybe just the set of instructions that explains how auto is awesome, and why shared_ptr construct is necessary to represent partial / shared ownership semantics, plus associated set of multi-threaded context propagation tests / examples.

I'm quite tempted to set the status on it to CHANGES REQUESTED ... unless we justify the perf impact / loss of partial ownership semantics.

lalitb · 2021-06-16T09:56:38Z

These are the perf numbers for baggage api ( the earlier one posted at #828 was for Span create api ) with and without PIMP idiom.

Existing Implementation ( without PIMPL):

CPU Caches:
  L1 Data 32K (x4)
  L1 Instruction 32K (x4)
  L2 Unified 256K (x4)
  L3 Unified 8192K (x1)
Load Average: 0.38, 0.44, 0.35
----------------------------------------------------------------------------
Benchmark                                  Time             CPU   Iterations
----------------------------------------------------------------------------
BM_CreateBaggageFromTenEntries         14671 ns        14671 ns        44127
BM_ExtractBaggageHavingTenEntries        361 ns          361 ns      1853059
BM_CreateBaggageFrom180Entries        271756 ns       271757 ns         2541
BM_ExtractBaggageWith180Entries         5815 ns         5815 ns       115602
BM_SetValueBaggageWithTenEntries        2344 ns         2344 ns       301103
BM_SetValueBaggageWith180Entries       42179 ns        42179 ns        16987
BM_BaggageToHeaderTenEntries            3992 ns         3992 ns       178561
BM_BaggageToHeader180Entries           69452 ns        69451 ns         9263

New Implementation ( this PR) using PIMPL

CPU Caches:
  L1 Data 32K (x4)
  L1 Instruction 32K (x4)
  L2 Unified 256K (x4)
  L3 Unified 8192K (x1)
Load Average: 0.39, 0.44, 0.35
----------------------------------------------------------------------------
Benchmark                                  Time             CPU   Iterations
----------------------------------------------------------------------------
BM_CreateBaggageFromTenEntries         13780 ns        13780 ns        45221
BM_ExtractBaggageHavingTenEntries        378 ns          378 ns      1886558
BM_CreateBaggageFrom180Entries        263451 ns       263452 ns         2631
BM_ExtractBaggageWith180Entries         5634 ns         5634 ns       113505
BM_SetValueBaggageWithTenEntries        2394 ns         2394 ns       298951
BM_SetValueBaggageWith180Entries       40432 ns        40432 ns        16882
BM_BaggageToHeaderTenEntries            3988 ns         3988 ns       173536
BM_BaggageToHeader180Entries           70706 ns        70706 ns         8979

There is no significant performance difference in both the approaches, which is expected as we still use shared_ptr<Baggage> internally as a data member in the PIMPL approach.
The benchmark tests are in PR #861.

return Baggage instead of shared_ptr<Baggage>

a662d19

lalitb requested a review from a team June 8, 2021 15:10

lalitb mentioned this pull request Jun 8, 2021

Can the API avoid returning shared_ptr<Span> #828

Open

lalitb changed the title ~~[POC] Baggage API : return Baggage instead of shared_ptr<Baggage> using Pimpl idiom~~ [POC] Baggage API : return Baggage instead of shared_ptr<Baggage> using PImpl idiom Jun 8, 2021

Merge branch 'main' into span_impl

cf5d2ce

maxgolov reviewed Jun 8, 2021

View reviewed changes

api/include/opentelemetry/baggage/baggage_impl.h Show resolved Hide resolved

pyohannes reviewed Jun 9, 2021

View reviewed changes

bogdandrutu reviewed Jun 9, 2021

View reviewed changes

lalitb added the pr:do-not-merge This PR is not ready to be merged. label Jun 10, 2021

Merge branch 'main' into span_impl

c6aaf37

github-actions bot added the Stale label Nov 11, 2021

github-actions bot closed this Nov 19, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[POC] Baggage API : return `Baggage` instead of `shared_ptr<Baggage>` using PImpl idiom #836

[POC] Baggage API : return `Baggage` instead of `shared_ptr<Baggage>` using PImpl idiom #836

lalitb commented Jun 8, 2021

codecov bot commented Jun 8, 2021 •

edited

Loading

jsuereth commented Jun 8, 2021

lalitb commented Jun 8, 2021

maxgolov Jun 8, 2021

lalitb Jun 9, 2021

pyohannes Jun 9, 2021

lalitb Jun 9, 2021 •

edited

Loading

maxgolov Jun 9, 2021

maxgolov Jun 9, 2021 •

edited

Loading

lalitb Jun 9, 2021

maxgolov Jun 9, 2021

pyohannes Jun 10, 2021

bogdandrutu left a comment

bogdandrutu Jun 9, 2021

maxgolov commented Jun 15, 2021 •

edited

Loading

lalitb commented Jun 16, 2021 •

edited

Loading

[POC] Baggage API : return Baggage instead of shared_ptr<Baggage> using PImpl idiom #836

[POC] Baggage API : return Baggage instead of shared_ptr<Baggage> using PImpl idiom #836

Conversation

lalitb commented Jun 8, 2021

codecov bot commented Jun 8, 2021 • edited Loading

Codecov Report

jsuereth commented Jun 8, 2021

lalitb commented Jun 8, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lalitb Jun 9, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maxgolov Jun 9, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bogdandrutu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maxgolov commented Jun 15, 2021 • edited Loading

lalitb commented Jun 16, 2021 • edited Loading

[POC] Baggage API : return `Baggage` instead of `shared_ptr<Baggage>` using PImpl idiom #836

[POC] Baggage API : return `Baggage` instead of `shared_ptr<Baggage>` using PImpl idiom #836

codecov bot commented Jun 8, 2021 •

edited

Loading

lalitb Jun 9, 2021 •

edited

Loading

maxgolov Jun 9, 2021 •

edited

Loading

maxgolov commented Jun 15, 2021 •

edited

Loading

lalitb commented Jun 16, 2021 •

edited

Loading