Context and shared ptr improvements #3713
Conversation
- prevent a superfluous copy of ContextValue - replace raw char* key pointer with std::string, increases sizeof(DataList) but introduces small string optimization - introduced rule of 0 as far as possible (copy/move constructor and assignment and also destructor can be generated by compiler)
- removed unnecessary virtual functions - may reduce sizeof(shared_ptr) by 50% (sizeof(shared_ptr) may be just 16 bytes - removed unused header cstdlib - rule of 0 (removed destructor) - replace unique_ptr.release() by std::move() to make it more clear/readable
|
missing header |
- removed another copy of ContextValue - made functions not changing internal const
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3713 +/- ##
==========================================
+ Coverage 90.00% 90.01% +0.01%
==========================================
Files 225 225
Lines 7099 7091 -8
==========================================
- Hits 6389 6382 -7
+ Misses 710 709 -1
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull Request Overview
This PR optimizes the Context and nostd::shared_ptr implementations by reducing memory overhead and eliminating dynamic memory allocations. The changes improve performance through more efficient memory management, removal of virtual function overhead, and enabling SSO (Small String Optimization) for context keys.
Key changes:
- Removes virtual functions from
shared_ptr_wrapperto eliminate vtable overhead - Uses
std::stringfor context keys instead of rawchar*to enable SSO and simplify memory management - Dynamically sizes
PlacementBufferbased onshared_ptr_wrappersize to potentially reducesizeof(shared_ptr)
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
api/include/opentelemetry/nostd/shared_ptr.h |
Removes virtual functions, dynamically sizes placement buffer, adds self-assignment checks, fixes unique_ptr conversion |
api/include/opentelemetry/context/context.h |
Replaces manual key memory management with std::string, simplifies key comparison, marks methods const-correct |
.vscode/launch.json |
Removes IDE-specific configuration file |
| shared_ptr(unique_ptr<T> &&other) noexcept | ||
| { | ||
| std::shared_ptr<T> ptr_(other.release()); | ||
| std::shared_ptr<T> ptr_(std::move(other)); |
There was a problem hiding this comment.
Using std::move() on a nostd::unique_ptr will not work correctly. The original code with other.release() was correct. nostd::unique_ptr may not have a move constructor that works with std::shared_ptr, but it does have a release() method that returns the raw pointer. This change will likely cause compilation errors or incorrect behavior.
| std::shared_ptr<T> ptr_(std::move(other)); | |
| std::shared_ptr<T> ptr_(other.release()); |
There was a problem hiding this comment.
I don't get this!
According to cppref(constructor 13) this constructor exists. Additionally nostd::unique_ptr implements move operations perfectly.
Also, if this would be true, then code like:
nostd::unique_ptr<T> Foo() { return new T; }
std::shared_ptr<T> ptr(Foo());
would not work.
PS: GPT-5 says everything is fine with this code
There was a problem hiding this comment.
As I understand it, std::shared_ptr should not recognize nostd::unique_ptr and cannot be constructed from its rvalue.
There was a problem hiding this comment.
As I understand it, std::shared_ptr should not recognize nostd::unique_ptr and cannot be constructed from its rvalue.
Perfect, I was blind and naive. Thought I saw a test for it, but didn't. After writing one I directly had compiler error.
| // and returns that head. | ||
| DataList(nostd::string_view key, const ContextValue &value) | ||
| : key_(key.begin(), key.end()) | ||
| , value_( value) |
There was a problem hiding this comment.
Extra space before closing parenthesis in initializer list.
| , value_( value) | |
| , value_(value) |
- Fixed formatting issues in `context.h` - Adjusted `shared_ptr` construction from `unique_ptr` - Added test for move construction from `nostd::unique_ptr`
Reneg973
force-pushed
the
Context_and_shared_ptr_improvements
branch
from
481649a to
923bc47
Compare
marcalff
left a comment
marcalff
left a comment
There was a problem hiding this comment.
Thanks for the PR.
This area is much more complex that is appears on first sight.
Both the API and the ABI here must be immutable, so that:
- an library compiled with opentelemetry-cpp API 1.23.0 or older
- another library compiled with opentelemetry-cpp API 1.24.0 or newer
- all linked in an application configured with possibly yet another SDK version
all agree on what bit in the binary layout of a context or shared_ptr means, and can operate on the same data.
If we were to implement this area from scratch today, we would probably use most of the cleanup done here, but unfortunately we can not, due ABI breaking changes.
We could implement some changes only in ABI_VERSION >= 2, but this is not worth the added complexity in my opinion.
Some areas in opentelemetry-cpp are easier and less risky than others, the API is deceptive, and probably the most difficult to change, compared to contributions in the SDK itself for example.
| // Creates a context object from a key and value, this will | ||
| // hold a shared_ptr to the head of the DataList linked list | ||
| Context(nostd::string_view key, ContextValue value) noexcept | ||
| Context(nostd::string_view key, const ContextValue &value) noexcept |
| // contains the new key and value data. It attaches the | ||
| // exisiting list to the end of the new list. | ||
| Context SetValue(nostd::string_view key, ContextValue value) noexcept | ||
| Context SetValue(nostd::string_view key, const ContextValue &value) const noexcept |
There was a problem hiding this comment.
Valid and better, but ABI break
| struct DataList | ||
| { | ||
| char *key_ = nullptr; | ||
| std::string key_; |
| virtual ~shared_ptr_wrapper() {} | ||
|
|
||
| virtual void CopyTo(PlacementBuffer &buffer) const noexcept | ||
| void CopyTo(PlacementBuffer &buffer) const noexcept |
There was a problem hiding this comment.
Valid and better, but ABI break.
| static_assert(sizeof(shared_ptr_wrapper) <= kMaxSize, "Placement buffer is too small"); | ||
| struct alignas(kAlignment) PlacementBuffer | ||
| { | ||
| char data[sizeof(shared_ptr_wrapper)]{}; |
| if (this == &other) | ||
| { | ||
| return *this; | ||
| } |
There was a problem hiding this comment.
This is valid, please post a separate PR for this fix,
Well, if you decide not to integrate it, you can close this PR, which is ok, even if I disagree.
Does this mean you want to allow use cases where an object is created with API 1.23 and pushed into 1.24? Otherwise I don't see that they influence each other. This was just a first proposal to slim down the overhead without losing functionality. |
The problem is as follows: For For I agree it would improve the final product, but it also creates a significant effort in maintenance in the mean time, with potential regressions if things are different between abi 1 and 2. This is what I meant with added complexity.
No contest here. The question is whether this is fixable without breaking existing applications, and without too much burden.
The API code has to be header only, which excludes using a custom allocator, memory pool, or anything like it. The SDK and exporters have more implementation freedom, but this is another area.
For example, see #828 This has been known for a long time, and is not trivial to fix, due to the breaking change label.
That sounds fixable in ABI_VERSION 2.
An application or a library is instrumented by invoking the API only. The SDK is what is configured in the main() for the overall application. It could be SDK 1.25, which implements the API. The problem here is that SDK 1.25 has to work not just with API 1.25, which is in the same code base in the same git label, but also with API 1.23. There is no CI build to verify this, so every time any code under api/include is touched, it needs extra review. Long story short, if a library is instrumented (with API 1.23) and ships a binary package, linking this library in an application that uses the SDK 1.25 should not require whoever provided the library to produce and ship again another package (built with API 1.25).
And this is appreciated. Overall, ABI 1 and 2 are already different, and CI is already covering both, so having a few more ifdef to fix existing issues in ABI 2 only is possible. Ultimately, this work is needed, and the sooner ABI 2 can be cleaned up and ready, the sooner we can deprecate ABI 1. The pitfall to avoid is to start making changes, then get distracted and delayed, to end up with ABI 1 still in use, ABI 2 very different, better but still not complete, with diverging code. Considering all the cleanup needed anyway due to recent clang-tidy findings in #3762, it looks like opentelemetry-cpp can not postpone this cleanup forever, so we might as well start it. Do you think the PR can be adjusted to change only ABI_VERSION 2, and pass all ci ? It will be considered. |
|
Your explanations are very appreciated, thanks.
ok, that's the kind of statement I waited for (getting a hint, what to do that this PR could be approved). |
4 participants
Fixes # (issue)
non, just improvements
Changes
non-API changes in
Contextandnostd::shared_ptrPlease provide a brief description of the changes here.
sizeof(nostd::shared_ptr)and thereforesizeof(Context), if sizeof(std::shared_ptr) == 16 instead of 32Please provide a brief description of the changes here.
For significant contributions please make sure you have completed the following items:
CHANGELOG.mdupdated for non-trivial changes