From 97245af04d19ee48b42d509f44fa77bdd17ceb0e Mon Sep 17 00:00:00 2001 From: Rene Greiner Date: Fri, 24 Oct 2025 11:04:39 +0200 Subject: [PATCH 1/5] Context improvements - 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) --- api/include/opentelemetry/context/context.h | 67 ++++----------------- 1 file changed, 12 insertions(+), 55 deletions(-) diff --git a/api/include/opentelemetry/context/context.h b/api/include/opentelemetry/context/context.h index 924036efad..ea8d281770 100644 --- a/api/include/opentelemetry/context/context.h +++ b/api/include/opentelemetry/context/context.h @@ -44,8 +44,8 @@ class Context template Context SetValues(T &values) noexcept { - Context context = Context(values); - nostd::shared_ptr last = context.head_; + Context context(values); + auto last = context.head_; while (last->next_ != nullptr) { last = last->next_; @@ -57,7 +57,7 @@ class Context // Accepts a new iterable and then returns a new context that // 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) noexcept { Context context = Context(key, value); context.head_->next_ = head_; @@ -69,12 +69,9 @@ class Context { for (DataList *data = head_.get(); data != nullptr; data = data->next_.get()) { - if (key.size() == data->key_length_) + if (key == data->key_) { - if (std::memcmp(key.data(), data->key_, data->key_length_) == 0) - { - return data->value_; - } + return data->value_; } } return ContextValue{}; @@ -92,12 +89,10 @@ class Context // A linked list to contain the keys and values of this context node struct DataList { - char *key_ = nullptr; + std::string key_; nostd::shared_ptr next_{nullptr}; - size_t key_length_ = 0UL; - ContextValue value_; DataList() = default; @@ -106,20 +101,13 @@ class Context template DataList(const T &keys_and_vals) { - bool first = true; auto *node = this; - for (auto &iter : keys_and_vals) + auto iter = std::begin(keys_and_vals); + *node = DataList(iter.first, iter.second); + for (++iter; iter != std::end(keys_and_vals); ++iter) { - if (first) - { - *node = DataList(iter.first, iter.second); - first = false; - } - else - { - node->next_ = nostd::shared_ptr(new DataList(iter.first, iter.second)); - node = node->next_.get(); - } + node->next_ = nostd::shared_ptr(new DataList(iter.first, iter.second)); + node = node->next_.get(); } } @@ -127,41 +115,10 @@ class Context // and returns that head. DataList(nostd::string_view key, const ContextValue &value) { - key_ = new char[key.size()]; - key_length_ = key.size(); - std::memcpy(key_, key.data(), key.size() * sizeof(char)); + key_.assign(key.begin(), key.end()); next_ = nostd::shared_ptr{nullptr}; value_ = value; } - - DataList(const DataList &other) - : key_(new char[other.key_length_]), - next_(other.next_), - key_length_(other.key_length_), - value_(other.value_) - { - std::memcpy(key_, other.key_, other.key_length_ * sizeof(char)); - } - - DataList &operator=(DataList &&other) noexcept - { - key_length_ = other.key_length_; - value_ = std::move(other.value_); - next_ = std::move(other.next_); - - key_ = other.key_; - other.key_ = nullptr; - - return *this; - } - - ~DataList() - { - if (key_ != nullptr) - { - delete[] key_; - } - } }; // Head of the list which holds the keys and values of this context From 6d03e1b0a4017f1d972924b8c4d216989eeffe4f Mon Sep 17 00:00:00 2001 From: Rene Greiner Date: Fri, 24 Oct 2025 12:40:01 +0200 Subject: [PATCH 2/5] shared_ptr improvements - 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 --- api/include/opentelemetry/nostd/shared_ptr.h | 35 +++++++++++--------- 1 file changed, 20 insertions(+), 15 deletions(-) diff --git a/api/include/opentelemetry/nostd/shared_ptr.h b/api/include/opentelemetry/nostd/shared_ptr.h index 681c4eb377..0c75011f7c 100644 --- a/api/include/opentelemetry/nostd/shared_ptr.h +++ b/api/include/opentelemetry/nostd/shared_ptr.h @@ -11,7 +11,6 @@ #endif #if !defined(OPENTELEMETRY_HAVE_STD_SHARED_PTR) -# include # include # include @@ -32,13 +31,9 @@ class shared_ptr using pointer = element_type *; private: - static constexpr size_t kMaxSize = 32; static constexpr size_t kAlignment = 8; - struct alignas(kAlignment) PlacementBuffer - { - char data[kMaxSize]{}; - }; + struct alignas(kAlignment) PlacementBuffer; // fwd. class shared_ptr_wrapper { @@ -47,14 +42,12 @@ class shared_ptr shared_ptr_wrapper(std::shared_ptr &&ptr) noexcept : ptr_{std::move(ptr)} {} - virtual ~shared_ptr_wrapper() {} - - virtual void CopyTo(PlacementBuffer &buffer) const noexcept + void CopyTo(PlacementBuffer &buffer) const noexcept { new (buffer.data) shared_ptr_wrapper{*this}; } - virtual void MoveTo(PlacementBuffer &buffer) noexcept + void MoveTo(PlacementBuffer &buffer) noexcept { new (buffer.data) shared_ptr_wrapper{std::move(this->ptr_)}; } @@ -67,15 +60,19 @@ class shared_ptr new (buffer.data) other_shared_ptr_wrapper{std::move(this->ptr_)}; } - virtual pointer Get() const noexcept { return ptr_.get(); } + pointer Get() const noexcept { return ptr_.get(); } - virtual void Reset() noexcept { ptr_.reset(); } + void Reset() noexcept { ptr_.reset(); } private: std::shared_ptr ptr_; }; - static_assert(sizeof(shared_ptr_wrapper) <= kMaxSize, "Placement buffer is too small"); + struct alignas(kAlignment) PlacementBuffer + { + char data[sizeof(shared_ptr_wrapper)]{}; + }; + static_assert(alignof(shared_ptr_wrapper) <= kAlignment, "Placement buffer not properly aligned"); public: @@ -105,13 +102,13 @@ class shared_ptr shared_ptr(unique_ptr &&other) noexcept { - std::shared_ptr ptr_(other.release()); + std::shared_ptr ptr_(std::move(other)); new (buffer_.data) shared_ptr_wrapper{std::move(ptr_)}; } shared_ptr(std::unique_ptr &&other) noexcept { - std::shared_ptr ptr_(other.release()); + std::shared_ptr ptr_(std::move(other)); new (buffer_.data) shared_ptr_wrapper{std::move(ptr_)}; } @@ -119,6 +116,10 @@ class shared_ptr shared_ptr &operator=(shared_ptr &&other) noexcept { + if (this == &other) + { + return *this; + } wrapper().~shared_ptr_wrapper(); other.wrapper().MoveTo(buffer_); return *this; @@ -132,6 +133,10 @@ class shared_ptr shared_ptr &operator=(const shared_ptr &other) noexcept { + if (this == &other) + { + return *this; + } wrapper().~shared_ptr_wrapper(); other.wrapper().CopyTo(buffer_); return *this; From 1ec0d32c6973ae8bf70ba26bb82e006f22bbaf14 Mon Sep 17 00:00:00 2001 From: Rene Greiner Date: Fri, 24 Oct 2025 20:32:15 +0200 Subject: [PATCH 3/5] fixed include and compiler error - removed another copy of ContextValue - made functions not changing internal const --- .vscode/launch.json | 33 --------------------- api/include/opentelemetry/context/context.h | 23 +++++++------- 2 files changed, 12 insertions(+), 44 deletions(-) delete mode 100644 .vscode/launch.json diff --git a/.vscode/launch.json b/.vscode/launch.json deleted file mode 100644 index c7ad6761dd..0000000000 --- a/.vscode/launch.json +++ /dev/null @@ -1,33 +0,0 @@ -{ - "version": "0.2.0", - "configurations": [ - { - "name": "(ctest) Launch", - "type": "cppdbg", - "cwd": "${cmake.testWorkingDirectory}", - "request": "launch", - "program": "${cmake.testProgram}", - "args": [ "${cmake.testArgs}" ], - // other options... - }, - { - "name": "Debug on Windows", - "type": "cppvsdbg", - "request": "launch", - "program": "${workspaceFolder}/build/", - "args": [], - "stopAtEntry": false, - "cwd": "${workspaceFolder}", - "environment": [], - "externalConsole": false - }, - { - "name": "Debug on Linux", - "type": "gdb", - "request": "launch", - "target": "${workspaceFolder}/bazel-bin/", - "cwd": "${workspaceRoot}", - "valuesFormatting": "parseText" - } - ] -} diff --git a/api/include/opentelemetry/context/context.h b/api/include/opentelemetry/context/context.h index ea8d281770..2331f32a59 100644 --- a/api/include/opentelemetry/context/context.h +++ b/api/include/opentelemetry/context/context.h @@ -3,7 +3,7 @@ #pragma once -#include +#include #include #include "opentelemetry/context/context_value.h" @@ -34,7 +34,7 @@ class Context // 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 : head_{nostd::shared_ptr{new DataList(key, value)}} {} @@ -42,7 +42,7 @@ class Context // contains the new key and value data. It attaches the // exisiting list to the end of the new list. template - Context SetValues(T &values) noexcept + Context SetValues(T &values) const noexcept { Context context(values); auto last = context.head_; @@ -57,9 +57,9 @@ class Context // Accepts a new iterable and then returns a new context that // 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, const ContextValue& value) noexcept + Context SetValue(nostd::string_view key, const ContextValue& value) const noexcept { - Context context = Context(key, value); + Context context(key, value); context.head_->next_ = head_; return context; } @@ -101,12 +101,14 @@ class Context template DataList(const T &keys_and_vals) { - auto *node = this; auto iter = std::begin(keys_and_vals); - *node = DataList(iter.first, iter.second); + if (iter == std::end(keys_and_vals)) + return; + auto *node = this; + *node = DataList(iter->first, iter->second); for (++iter; iter != std::end(keys_and_vals); ++iter) { - node->next_ = nostd::shared_ptr(new DataList(iter.first, iter.second)); + node->next_ = nostd::shared_ptr(new DataList(iter->first, iter->second)); node = node->next_.get(); } } @@ -114,10 +116,9 @@ class Context // Builds a data list with just a key and value, so it will just be the head // and returns that head. DataList(nostd::string_view key, const ContextValue &value) + : key_(key.begin(), key.end()) + , value_( value) { - key_.assign(key.begin(), key.end()); - next_ = nostd::shared_ptr{nullptr}; - value_ = value; } }; From 000b21e471ef11960128d405f6b49590ac01a0e3 Mon Sep 17 00:00:00 2001 From: Rene Greiner Date: Sat, 25 Oct 2025 13:50:33 +0200 Subject: [PATCH 4/5] try to fix formatting complaints --- api/include/opentelemetry/context/context.h | 10 ++++------ api/include/opentelemetry/nostd/shared_ptr.h | 2 +- 2 files changed, 5 insertions(+), 7 deletions(-) diff --git a/api/include/opentelemetry/context/context.h b/api/include/opentelemetry/context/context.h index 2331f32a59..12678cbc96 100644 --- a/api/include/opentelemetry/context/context.h +++ b/api/include/opentelemetry/context/context.h @@ -57,7 +57,7 @@ class Context // Accepts a new iterable and then returns a new context that // 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, const ContextValue& value) const noexcept + Context SetValue(nostd::string_view key, const ContextValue &value) const noexcept { Context context(key, value); context.head_->next_ = head_; @@ -105,7 +105,7 @@ class Context if (iter == std::end(keys_and_vals)) return; auto *node = this; - *node = DataList(iter->first, iter->second); + *node = DataList(iter->first, iter->second); for (++iter; iter != std::end(keys_and_vals); ++iter) { node->next_ = nostd::shared_ptr(new DataList(iter->first, iter->second)); @@ -116,10 +116,8 @@ class Context // Builds a data list with just a key and value, so it will just be the head // and returns that head. DataList(nostd::string_view key, const ContextValue &value) - : key_(key.begin(), key.end()) - , value_( value) - { - } + : key_(key.begin(), key.end()), value_(value) + {} }; // Head of the list which holds the keys and values of this context diff --git a/api/include/opentelemetry/nostd/shared_ptr.h b/api/include/opentelemetry/nostd/shared_ptr.h index 0c75011f7c..0defd270d8 100644 --- a/api/include/opentelemetry/nostd/shared_ptr.h +++ b/api/include/opentelemetry/nostd/shared_ptr.h @@ -33,7 +33,7 @@ class shared_ptr private: static constexpr size_t kAlignment = 8; - struct alignas(kAlignment) PlacementBuffer; // fwd. + struct alignas(kAlignment) PlacementBuffer; // fwd. class shared_ptr_wrapper { From 923bc4735964d96ac3e474d0169ed43c8e4babdc Mon Sep 17 00:00:00 2001 From: Rene Greiner Date: Sun, 26 Oct 2025 16:50:12 +0100 Subject: [PATCH 5/5] fixed shared_ptr constructor from unique_ptr and context formatting - Fixed formatting issues in `context.h` - Adjusted `shared_ptr` construction from `unique_ptr` - Added test for move construction from `nostd::unique_ptr` --- api/include/opentelemetry/context/context.h | 4 ++-- api/include/opentelemetry/nostd/shared_ptr.h | 2 +- api/test/nostd/shared_ptr_test.cc | 10 ++++++++++ 3 files changed, 13 insertions(+), 3 deletions(-) diff --git a/api/include/opentelemetry/context/context.h b/api/include/opentelemetry/context/context.h index 12678cbc96..a8c5fd2a91 100644 --- a/api/include/opentelemetry/context/context.h +++ b/api/include/opentelemetry/context/context.h @@ -105,7 +105,7 @@ class Context if (iter == std::end(keys_and_vals)) return; auto *node = this; - *node = DataList(iter->first, iter->second); + *node = DataList(iter->first, iter->second); for (++iter; iter != std::end(keys_and_vals); ++iter) { node->next_ = nostd::shared_ptr(new DataList(iter->first, iter->second)); @@ -116,7 +116,7 @@ class Context // Builds a data list with just a key and value, so it will just be the head // and returns that head. DataList(nostd::string_view key, const ContextValue &value) - : key_(key.begin(), key.end()), value_(value) + : key_(key.begin(), key.end()), value_(value) {} }; diff --git a/api/include/opentelemetry/nostd/shared_ptr.h b/api/include/opentelemetry/nostd/shared_ptr.h index 0defd270d8..8d910dae13 100644 --- a/api/include/opentelemetry/nostd/shared_ptr.h +++ b/api/include/opentelemetry/nostd/shared_ptr.h @@ -102,7 +102,7 @@ class shared_ptr shared_ptr(unique_ptr &&other) noexcept { - std::shared_ptr ptr_(std::move(other)); + std::shared_ptr ptr_(other.release()); new (buffer_.data) shared_ptr_wrapper{std::move(ptr_)}; } diff --git a/api/test/nostd/shared_ptr_test.cc b/api/test/nostd/shared_ptr_test.cc index fa3d97c493..505310206b 100644 --- a/api/test/nostd/shared_ptr_test.cc +++ b/api/test/nostd/shared_ptr_test.cc @@ -9,6 +9,7 @@ #include #include "opentelemetry/nostd/shared_ptr.h" +#include "opentelemetry/nostd/unique_ptr.h" using opentelemetry::nostd::shared_ptr; @@ -96,6 +97,15 @@ TEST(SharedPtrTest, MoveConstructionFromStdSharedPtr) EXPECT_EQ(ptr2.get(), value); } +TEST(SharedPtrTest, MoveConstructionFromNoStdUniquePtr) +{ + opentelemetry::v1::nostd::unique_ptr value(new int{123}); + auto p = value.get(); + shared_ptr ptr{std::move(value)}; + EXPECT_EQ(value.get(), nullptr); // NOLINT + EXPECT_EQ(ptr.get(), p); +} + TEST(SharedPtrTest, Destruction) { bool was_destructed;