Can the API avoid returning shared_ptr<Span>

[bogdandrutu]

See: https://github.com/open-telemetry/opentelemetry-cpp/blob/main/api/include/opentelemetry/trace/tracer.h#L39

By having this design an atomic ref-counting will always be needed. This will block in the future a possibly more performant "streaming" implementation where Span == SpanContext and all "record" apis will stream an event. The reason for "blocking" is because will still pay the refcounting and heap allocation cost even though is not needed.

The SDK can implement the Span by having an internal shared_ptr using a pimpl pattern.

[bogdandrutu]

Same comment should apply for "Baggage" as well. It should allow an alternative implementation (even though is not a proper implementation but could be a header replace) that does not require Baggage to be heap allocated and refcounted.

[jsuereth]

We (Google) would be a fan of this change, giving us freedom to control performance going forward with differing implementations

[pyohannes]

This should be doable.

I'm not sure if we can preserve the ability to automatically end a span when it is deallocated (or goes out of scope). We might end up requiring explicit calls to End for all spans that are not managed by a Scope.

[bogdandrutu]

You can achieve exactly the same behavior by having internally a shared_ptr<SpanImpl> and have the copy ctor/assignment do ref and destructor unref, and when out of scope you call End. It is just a variant that does not change the behavior but changes the ownership of the reference object, and allows alternative implementations that can avoid using reference counting.

[lalitb]

Had a quick look into span api code, the problem I see here is that api::Span is an abstract class, which means we can't return a copy of it, so we have to return a reference or pointer of it ( holding the instance of sdk::Span), and this will introduce the typical ownership/memory issues where application/instrumentation-code is still holding the reference/pointer to object which has been already deleted.

For the rest of the components like Baggage / TraceState, we can use the Pimpl idiom to get rid of returning shared_ptr as implemented here: #836

[lalitb]

Just to add on top of the above comment, while we can add a non-abstract Span at API surface which internally contains a pointer to sdk::Span, and return this instead of shared_ptr<Span>. But this will make it difficult to maintain ABI compatibility. Any changes in internal structure of this non-abstract Span can break ABI compatibility. Returning shared_ptr allows us more flexibility to make internal changes without breaking ABI. Same holds true for Baggage api.

[ThomsonTan]

Could we add a new API to support returning pointer/reference to Span? Even we don't guarantee backward compatibility, we could try to not break existing customer.

[pyohannes]

One possible approach for this, which can be implemented very easily:

Return api::Span as a unique_ptr when a span is started, only convert it into a shared_ptr if it is set as the currently active span in the context. Because only in this case, the ownership of the span is truly shared (one copy is owned by the context, another by the user). With this solution users who want to benefit from the more efficient approach mustn't use automated context management and parenting.

Anyway, if we decide to go without using shared_ptr we'd need to start doing some prototyping, as the devil is in the details.

[bogdandrutu]

@lalitb can you please explain a bit more

But this will make it difficult to maintain ABI compatibility. Any changes in internal structure of this non-abstract Span can break ABI compatibility. Returning shared_ptr allows us more flexibility to make internal changes without breaking ABI. Same holds true for Baggage api.

What is the difference in your baggage prototype from ABI compatibility? If you change the current Baggage class is it not the same?

[lalitb]

What is the difference in your baggage prototype from ABI compatibility? If you change the current Baggage class is it not the same?

Returning Baggage as an object exposes the memory layout of the object to the caller, and the caller can start having a dependency on this memory layout which can break ABI compatibility. As an example, if the caller is using sizeof(Baggage) in their code, and we add a new private data member in the Baggage class, this will break the caller code. Returning shared_ptr<Baggage> won't give caller this flexibility.

Though you are correct for the baggage prototype, as most of the implementation changes would happen in BaggageImpl, which will not break the ABI compatibility.

[maxgolov]

Can we have an implementation that performs the following:

• using SpanValue = nostd::shared_ptr<Span> - to maintain backwards compatibility with v1.0.0-rc1 API. I already have customers using this API surface since v0.6... Then return that. And alternate implementation may decide how to override that implementation of SpanValue with a better performing implementation.

• perf concern is somewhat exaggerated for spans with "reasonable" duration, i.e. not a synthetic benchmark that emits 0-microseconds spans, but spans that actually perform some measurable amount of work.. Otherwise for immeasurable amount of work less than 0ms, I'd argue the instrumentation is unnecessary. You'd want to monitor and optimize the hot-path that is consuming most CPU cycles, not overburden it with unnecessary span creation? It'd be hard to measure the perf difference on non-synthetic benchmark. I'd like to see the concrete numbers from Google Benchmark. Something like this, but in a loop that constitutes some reasonable amount of work.

• then assuming we have SpanValue, as value object that possibly uses PIMPL to wrap a handle (I was always wondering why we use shared_ptr instead of returning something that is a handle), we could have allowed the alternate implementations that are better optimized for the streaming scenario.

By the way, ETW exporter is a "streaming" implementation. We can do measurements to tell how big the overhead is of the shared_ptr. ETW exporter / TracerProvider implementation does not use a batcher / background thread, it's a pass-thru of all spans and events to ETW layer, that allows to reassemble and/or filter the spans and events in batches by out-of-proc agent listener.

My biggest concern is refactoring a major piece something that reached v1.0.0-rc1. I'd like to see some smooth transition to a better model instead of a breaking change. If SpanValue is not a good name, let's come up with a better name. But at least tackle this in a non-breaking, incremental, fashion.

[lalitb]

The solution would depend on what we are trying to achieve here by removing ABI stable smart-pointer from API surface (sorry but I am still trying to get more clarity):

• performance improvement - It would be interesting to see if we can achieve performance improvement by removing shared_ptr from the API surface. As Span object would be shared at multiple places, we would internally still need a way to manage the reference count of this object ( either using pimpl approach by storing shared_ptr<SpanImpl> as data member or some other way) In fact, this may add marginal cost for another layer of access redirection.

• prevent unsafe smart pointer operations - If we want to prevent instrumentation libraries do unconscious unsafe operations with this smart pointer. Eg, trying to delete the managed raw pointer explicitly making the application crash, it makes sense to hide smart_pointer from the API surface. Though this will break backward compatibility.

[lalitb]

Ok, invested some time in prototyping span creation api to return unique_ptr<Span> instead of shared_ptr<Spam> here. Here are the benchmark results in identical load conditions:

Existing api ( using shared_ptr):

Run on (8 X 2112 MHz CPU s)
CPU Caches:
  L1 Data 32K (x4)
  L1 Instruction 32K (x4)
  L2 Unified 256K (x4)
  L3 Unified 8192K (x1)
Load Average: 0.09, 0.06, 0.02
------------------------------------------------------------------------------------------
Benchmark                                                Time             CPU   Iterations
------------------------------------------------------------------------------------------
BM_SpanCreation                                        266 ns          266 ns      2665824
BM_SpanCreationWithScope                              2227 ns         2227 ns       310808
BM_NestedSpanCreationWithScope                        7332 ns         7332 ns        91774
BM_SpanCreationWithManualSpanContextPropagation       1110 ns         1110 ns       626839
BM_SpanCreationWitContextPropagation                  6608 ns         6608 ns       103034

new api returning unique_ptr<Span> and using span's raw pointer internally to manage span context:

Run on (8 X 2112 MHz CPU s)
CPU Caches:
  L1 Data 32K (x4)
  L1 Instruction 32K (x4)
  L2 Unified 256K (x4)
  L3 Unified 8192K (x1)
Load Average: 0.10, 0.07, 0.02
------------------------------------------------------------------------------------------
Benchmark                                                Time             CPU   Iterations
------------------------------------------------------------------------------------------
BM_SpanCreation                                        468 ns          468 ns      1073888
BM_SpanCreationWithScope                              2270 ns         2270 ns       310252
BM_NestedSpanCreationWithScope                        7384 ns         7384 ns        92055
BM_SpanCreationWithManualSpanContextPropagation       1486 ns         1486 ns       463668
BM_SpanCreationWitContextPropagation                  6435 ns         6435 ns       103529

After doing multiple iterations, haven't seen a significant difference in performance in both scenarios. As we get rid of shared_ptr, there is extra overhead to manage the cleanup of Span from Context once Span gets deleted.

Another thing to notice is that there is a significant performance difference between Span creation with and without Scope (first two readings taken from either of the above scenarios), and it increased further with nested spans ( 3 reading above). This means we should revisit our existing linked-list / stack data structure used to manage context/spans. I will create a separate issue to investigate that, We can discuss this in the next community meeting.

ps. If you would like to review the changes done for conversion to unique_ptr: https://github.com/lalitb/opentelemetry-cpp/compare/main...lalitb:span-pointer?expand=1 ,relevant changes are confined to these files:
api/include/opentelemetry/context/context_value.h
api/include/opentelemetry/trace/scope.h
api/include/opentelemetry/trace/span.h
api/include/opentelemetry/trace/tracer.h

Code used for benchmarking:
https://github.com/open-telemetry/opentelemetry-cpp/blob/main/api/test/trace/span_benchmark.cc

[maxgolov]

So if the new proposed approach degrades perf of span creation x1.75 times, and/or does not improve performance overall - for the sum across all tests (not a good metric though), is worse - because of BM_SpanCreation and BM_SpanCreationWithManualSpanContextPropagation , this kinda defeats the purpose, or disproves the actuality of original expectations?

The reason for "blocking" is because will still pay the refcounting and heap allocation cost even though is not needed.

Why would we be doing that again? I think shared_ptr semantics allow us to pass Span objects to worker threads, properly representing partial ownership and safe disposal of the objects if- and when- needed. Performance point is clearly moot to say the least. auto keyword addresses most inconveniences. And conversion to unique_ptr does not yield any perf improvements, while taking away the partial ownership semantics.. I think it's not good, esp. when v1.0 API is stable, working, and many people are using it / trying it out successfully.