From e9d9646bd3d120f2880c1c6fbca5d0df18da663a Mon Sep 17 00:00:00 2001 From: Gregory LEOCADIE Date: Thu, 7 Mar 2024 11:39:55 +0100 Subject: [PATCH] Use a homemade implementation of linked-list --- .../Datadog.Profiler.Native/CollectorBase.h | 9 +- .../Datadog.Profiler.Native.vcxproj | 1 + .../Datadog.Profiler.Native.vcxproj.filters | 8 +- .../Datadog.Profiler.Native/LinkedList.hpp | 209 +++++++++++++++ .../ManagedThreadInfo.h | 2 +- .../Datadog.Profiler.Native/RawSamples.hpp | 21 +- .../Datadog.Profiler.Native.Tests.vcxproj | 9 +- ...adog.Profiler.Native.Tests.vcxproj.filters | 5 +- .../LinkedListTest.cpp | 251 ++++++++++++++++++ 9 files changed, 490 insertions(+), 25 deletions(-) create mode 100644 profiler/src/ProfilerEngine/Datadog.Profiler.Native/LinkedList.hpp create mode 100644 profiler/test/Datadog.Profiler.Native.Tests/LinkedListTest.cpp diff --git a/profiler/src/ProfilerEngine/Datadog.Profiler.Native/CollectorBase.h b/profiler/src/ProfilerEngine/Datadog.Profiler.Native/CollectorBase.h index 69f581172bae..98582f781455 100644 --- a/profiler/src/ProfilerEngine/Datadog.Profiler.Native/CollectorBase.h +++ b/profiler/src/ProfilerEngine/Datadog.Profiler.Native/CollectorBase.h @@ -88,7 +88,7 @@ class CollectorBase void Add(TRawSample&& sample) override { - _collectedSamples.Add(std::forward(sample)); + _collectedSamples.Add(std::move(sample)); } void TransformRawSample(const TRawSample& rawSample, std::shared_ptr& sample) @@ -150,9 +150,8 @@ class CollectorBase { public: SamplesEnumeratorImpl(RawSamples rawSamples, CollectorBase* collector) : - _rawSamples{std::move(rawSamples)}, _collector{collector} + _rawSamples{std::move(rawSamples)}, _collector{collector}, _currentRawSample{_rawSamples.begin()} { - _currentRawSample = _rawSamples.cbegin(); } // Inherited via SamplesEnumerator @@ -163,7 +162,7 @@ class CollectorBase bool MoveNext(std::shared_ptr& sample) override { - if (_currentRawSample == _rawSamples.cend()) + if (_currentRawSample == _rawSamples.end()) return false; _collector->TransformRawSample(*_currentRawSample, sample); @@ -175,7 +174,7 @@ class CollectorBase private: RawSamples _rawSamples; CollectorBase* _collector; - typename RawSamples::const_iterator _currentRawSample; + typename RawSamples::iterator _currentRawSample; }; private: diff --git a/profiler/src/ProfilerEngine/Datadog.Profiler.Native/Datadog.Profiler.Native.vcxproj b/profiler/src/ProfilerEngine/Datadog.Profiler.Native/Datadog.Profiler.Native.vcxproj index 036dd8d1e0d9..795fdb1f2a8e 100644 --- a/profiler/src/ProfilerEngine/Datadog.Profiler.Native/Datadog.Profiler.Native.vcxproj +++ b/profiler/src/ProfilerEngine/Datadog.Profiler.Native/Datadog.Profiler.Native.vcxproj @@ -246,6 +246,7 @@ + diff --git a/profiler/src/ProfilerEngine/Datadog.Profiler.Native/Datadog.Profiler.Native.vcxproj.filters b/profiler/src/ProfilerEngine/Datadog.Profiler.Native/Datadog.Profiler.Native.vcxproj.filters index 2e19e103ad0b..99e8a7f71876 100644 --- a/profiler/src/ProfilerEngine/Datadog.Profiler.Native/Datadog.Profiler.Native.vcxproj.filters +++ b/profiler/src/ProfilerEngine/Datadog.Profiler.Native/Datadog.Profiler.Native.vcxproj.filters @@ -431,9 +431,9 @@ Threads - - Utils - + + + @@ -653,4 +653,4 @@ - + \ No newline at end of file diff --git a/profiler/src/ProfilerEngine/Datadog.Profiler.Native/LinkedList.hpp b/profiler/src/ProfilerEngine/Datadog.Profiler.Native/LinkedList.hpp new file mode 100644 index 000000000000..4c218bd7022c --- /dev/null +++ b/profiler/src/ProfilerEngine/Datadog.Profiler.Native/LinkedList.hpp @@ -0,0 +1,209 @@ +// Unless explicitly stated otherwise all files in this repository are licensed under the Apache 2 License. +// This product includes software developed at Datadog (https://www.datadoghq.com/). Copyright 2022 Datadog, Inc. + +#pragma once + +#ifdef __has_include // Check if __has_include is present +#if __has_include() // Check for a standard library +#include +namespace pmr { +using namespace std::pmr; +} +#elif __has_include() // Check for an experimental version +#include +namespace pmr { +using namespace std::experimental::pmr; +} +#else // Not found at all +#error "Missing " +#endif +#endif + +#include +#include +#include +#include +#include + + +template +class LinkedList +{ +private: + struct Node; + using node_type = Node; + +public: + LinkedList(pmr::memory_resource* allocator = pmr::get_default_resource()) noexcept : + _head{nullptr}, _tail{&_head}, _nbElements{0}, _allocator{allocator} + { + } + + ~LinkedList() + { + auto* current = std::exchange(_head, nullptr); + + while (current != nullptr) + { + auto* next = current->Next; + std::destroy_at(current); + _allocator->deallocate(current, sizeof(Node)); + current = next; + } + } + + LinkedList(LinkedList const&) = delete; + LinkedList& operator=(LinkedList const&) = delete; + + LinkedList(LinkedList&& other) noexcept : + LinkedList() // should we use the default allocator ? or allow an allocator to the move ctor ? + { + *this = std::move(other); + } + + LinkedList& operator=(LinkedList&& other) noexcept + { + if (this == &other) + { + return *this; + } + + _head = std::exchange(other._head, nullptr); + _tail = _head == nullptr ? &_head : std::exchange(other._tail, nullptr); + +#ifdef DD_TEST + if (_head == nullptr && _tail != &_head) + { + throw std::exception("_tail must have the address of _head"); + } +#endif + std::swap(_nbElements, other._nbElements); + std::swap(_allocator, other._allocator); + + return *this; + } + + void swap(LinkedList& other) + { + if (this == &other) + { + return; + } + + _head = std::exchange(other._head, _head); + _tail = _head == nullptr ? &_head : other._tail; + other._tail = other._head == nullptr ? &other._head : _tail; + +#ifdef DD_TEST + if (_head == nullptr && _tail != &_head) + { + throw std::exception("_tail must have the address of _head"); + } + + if (other._head == nullptr && other._tail != &other._head) + { + throw std::exception("other._tail must have the address of other._head"); + } +#endif + + std::swap(_nbElements, other._nbElements); + std::swap(_allocator, other._allocator); + } + + bool append(T&& v) + { + auto ptr = _allocator->allocate(sizeof(Node)); + + if (ptr == nullptr) + { + return false; + } + + *_tail = new (ptr) Node(std::move(v), nullptr); + _tail = &((*_tail)->Next); + + _nbElements++; + + return true; + } + + std::size_t size() const + { + return _nbElements; + } + + class iterator; + + iterator begin() + { + return iterator(_head); + } + + iterator end() + { + return iterator(nullptr); + } + + class iterator + { + public: + iterator(Node* node) : + _Ptr{node} + { + } + + T& operator*() const + { + return _Ptr->Value; + } + + iterator operator++(int) + { + auto tmp = *this; + ++*this; + return tmp; + } + + iterator& operator++() + { + _Ptr = _Ptr->Next; + return *this; + } + + bool operator==(iterator const& other) const + { + return _Ptr == other._Ptr; + } + + bool operator!=(iterator const& other) const + { + return !(*this == other); + } + + private: + Node* _Ptr; + }; + +private: + struct Node + { + public: + T Value; + Node* Next; + + template + Node(Ty&& v, Node* next) : + Value(std::forward(v)), Next{next} + { + } + + Node(Node const&) = delete; + Node& operator=(Node const&) = delete; + }; + + Node* _head; + Node** _tail; + std::size_t _nbElements; + + pmr::memory_resource* _allocator; +}; \ No newline at end of file diff --git a/profiler/src/ProfilerEngine/Datadog.Profiler.Native/ManagedThreadInfo.h b/profiler/src/ProfilerEngine/Datadog.Profiler.Native/ManagedThreadInfo.h index 33863184a062..2562bd171554 100644 --- a/profiler/src/ProfilerEngine/Datadog.Profiler.Native/ManagedThreadInfo.h +++ b/profiler/src/ProfilerEngine/Datadog.Profiler.Native/ManagedThreadInfo.h @@ -414,4 +414,4 @@ inline void ManagedThreadInfo::SetSharedMemory(volatile int* memoryArea) { _sharedMemoryArea = memoryArea; } -#endif \ No newline at end of file +#endif diff --git a/profiler/src/ProfilerEngine/Datadog.Profiler.Native/RawSamples.hpp b/profiler/src/ProfilerEngine/Datadog.Profiler.Native/RawSamples.hpp index 301858ba7d39..b210713b15b4 100644 --- a/profiler/src/ProfilerEngine/Datadog.Profiler.Native/RawSamples.hpp +++ b/profiler/src/ProfilerEngine/Datadog.Profiler.Native/RawSamples.hpp @@ -3,13 +3,14 @@ // This product includes software developed at Datadog (https://www.datadoghq.com/). Copyright 2022 Datadog, Inc. #pragma once -#include + +#include "LinkedList.hpp" template class RawSamples { public: - using const_iterator = typename std::list::const_iterator; + using iterator = typename LinkedList::iterator; RawSamples() = default; @@ -31,7 +32,7 @@ class RawSamples { std::lock_guard lock(_lock); - std::list result; + LinkedList result; _samples.swap(result); return RawSamples(std::move(result)); @@ -40,17 +41,17 @@ class RawSamples void Add(TRawSample&& sample) { std::lock_guard lock(_lock); - _samples.push_back(std::forward(sample)); + _samples.append(std::forward(sample)); } - auto cbegin() const + auto begin() { - return _samples.cbegin(); + return _samples.begin(); } - auto cend() const + auto end() { - return _samples.cend(); + return _samples.end(); } std::size_t size() const @@ -59,11 +60,11 @@ class RawSamples } private: - RawSamples(std::list samples) : + RawSamples(LinkedList samples) : _samples{std::move(samples)} { } std::mutex _lock; - std::list _samples; + LinkedList _samples; }; \ No newline at end of file diff --git a/profiler/test/Datadog.Profiler.Native.Tests/Datadog.Profiler.Native.Tests.vcxproj b/profiler/test/Datadog.Profiler.Native.Tests/Datadog.Profiler.Native.Tests.vcxproj index 47696cacf58d..33c31109f817 100644 --- a/profiler/test/Datadog.Profiler.Native.Tests/Datadog.Profiler.Native.Tests.vcxproj +++ b/profiler/test/Datadog.Profiler.Native.Tests/Datadog.Profiler.Native.Tests.vcxproj @@ -63,6 +63,7 @@ + @@ -113,7 +114,7 @@ NotUsing pch.h Disabled - _SILENCE_STDEXT_ARR_ITERS_DEPRECATION_WARNING;WIN32;_DEBUG;_CONSOLE;_WINDOWS;%(PreprocessorDefinitions) + _SILENCE_STDEXT_ARR_ITERS_DEPRECATION_WARNING;DD_TEST;WIN32;_DEBUG;_CONSOLE;_WINDOWS;%(PreprocessorDefinitions) EnableFastChecks MultiThreadedDebug Level3 @@ -136,7 +137,7 @@ NotUsing pch.h Disabled - _SILENCE_STDEXT_ARR_ITERS_DEPRECATION_WARNING;X64;_DEBUG;_CONSOLE;_WINDOWS;%(PreprocessorDefinitions) + _SILENCE_STDEXT_ARR_ITERS_DEPRECATION_WARNING;DD_TEST;X64;_DEBUG;_CONSOLE;_WINDOWS;%(PreprocessorDefinitions) EnableFastChecks MultiThreadedDebug Level3 @@ -157,7 +158,7 @@ NotUsing pch.h - _SILENCE_STDEXT_ARR_ITERS_DEPRECATION_WARNING;WIN32;NDEBUG;_CONSOLE;_WINDOWS;%(PreprocessorDefinitions) + _SILENCE_STDEXT_ARR_ITERS_DEPRECATION_WARNING;DD_TEST;WIN32;NDEBUG;_CONSOLE;_WINDOWS;%(PreprocessorDefinitions) MultiThreaded Level3 ProgramDatabase @@ -180,7 +181,7 @@ NotUsing pch.h - _SILENCE_STDEXT_ARR_ITERS_DEPRECATION_WARNING;X64;NDEBUG;_CONSOLE;_WINDOWS;%(PreprocessorDefinitions) + _SILENCE_STDEXT_ARR_ITERS_DEPRECATION_WARNING;DD_TEST;X64;NDEBUG;_CONSOLE;_WINDOWS;%(PreprocessorDefinitions) MultiThreaded Level3 ProgramDatabase diff --git a/profiler/test/Datadog.Profiler.Native.Tests/Datadog.Profiler.Native.Tests.vcxproj.filters b/profiler/test/Datadog.Profiler.Native.Tests/Datadog.Profiler.Native.Tests.vcxproj.filters index 63b1470c8754..beed868dcdb0 100644 --- a/profiler/test/Datadog.Profiler.Native.Tests/Datadog.Profiler.Native.Tests.vcxproj.filters +++ b/profiler/test/Datadog.Profiler.Native.Tests/Datadog.Profiler.Native.Tests.vcxproj.filters @@ -157,6 +157,9 @@ Helpers + + Tests + @@ -188,4 +191,4 @@ Helpers - + \ No newline at end of file diff --git a/profiler/test/Datadog.Profiler.Native.Tests/LinkedListTest.cpp b/profiler/test/Datadog.Profiler.Native.Tests/LinkedListTest.cpp new file mode 100644 index 000000000000..4fec6e7ea096 --- /dev/null +++ b/profiler/test/Datadog.Profiler.Native.Tests/LinkedListTest.cpp @@ -0,0 +1,251 @@ + +#include "LinkedList.hpp" + +#include "gtest/gtest.h" + +TEST(LinkedListTest, Append) +{ + LinkedList x; + + ASSERT_EQ(x.size(), 0); + + ASSERT_TRUE(x.append(42)); + + ASSERT_EQ(x.size(), 1); + + ASSERT_EQ(*(x.begin()), 42); +} + +TEST(LinkedListTest, ForEach) +{ + LinkedList x; + + ASSERT_TRUE(x.append("1")); + ASSERT_TRUE(x.append("2")); + ASSERT_TRUE(x.append("3")); + + int i = 1; + + for (auto const& s : x) + { + ASSERT_EQ(std::to_string(i++), s); + } + + ASSERT_EQ(x.size(), 3); +} + +struct Person +{ + std::string Name; + std::uint8_t Age; +}; + +TEST(LinkedListTest, Move) +{ + LinkedList l; + + l.append({.Name = "Georges", .Age = 42}); + l.append({.Name = "Ralph", .Age = 101}); + l.append({.Name = "jordy", .Age = 21}); + l.append({.Name = "Bob", .Age = 1}); + + ASSERT_EQ(l.size(), 4); + + LinkedList other = std::move(l); + + ASSERT_EQ(0, l.size()); + + ASSERT_EQ(other.size(), 4); + + auto it = other.begin(); + + ASSERT_EQ((*it).Name, "Georges"); + ASSERT_EQ((*it).Age, 42); + + ++it; + + ASSERT_EQ((*it).Name, "Ralph"); + ASSERT_EQ((*it).Age, 101); + + ++it; + + ASSERT_EQ((*it).Name, "jordy"); + ASSERT_EQ((*it).Age, 21); + + ++it; + + ASSERT_EQ((*it).Name, "Bob"); + ASSERT_EQ((*it).Age, 1); +} + +TEST(LinkedListTest, Assign) +{ + LinkedList l; + + l.append({.Name = "Georges", .Age = 42}); + l.append({.Name = "Ralph", .Age = 101}); + l.append({.Name = "jordy", .Age = 21}); + l.append({.Name = "Bob", .Age = 1}); + + + + ASSERT_EQ(l.size(), 4); + + LinkedList other; + + other = std::move(l); + + // in this case, l will contains the default values + ASSERT_EQ(l.size(), 0); + + auto begin_it = l.begin(); + auto end_it = l.end(); + ASSERT_EQ(begin_it, end_it); + + // check other now + ASSERT_EQ(other.size(), 4); + + auto it = other.begin(); + + ASSERT_EQ((*it).Name, "Georges"); + ASSERT_EQ((*it).Age, 42); + + ++it; + + ASSERT_EQ((*it).Name, "Ralph"); + ASSERT_EQ((*it).Age, 101); + + ++it; + + ASSERT_EQ((*it).Name, "jordy"); + ASSERT_EQ((*it).Age, 21); + + ++it; + + ASSERT_EQ((*it).Name, "Bob"); + ASSERT_EQ((*it).Age, 1); +} + +TEST(LinkedListTest, ObjectDtorCalled) +{ + // Dummy class is moveable-only. + // We make sure that we do not set _dtorCalled field to true when destroying temporary instances + // but only only one instance. + class Dummy + { + public: + Dummy(bool* dtorCalled) : + _dtorCalled{dtorCalled} + { + } + + ~Dummy() + { + if (_dtorCalled != nullptr) + (*_dtorCalled) = true; + } + + Dummy(Dummy const&) = delete; + Dummy& operator=(Dummy const&) = delete; + + // We have to write the move ctor and assignement operator to make sure + // that we set the field on the single instance and not on temporary instances + Dummy(Dummy&& other) noexcept + { + *this = std::move(other); + } + + Dummy& operator=(Dummy&& other) noexcept + { + if (this == &other) + { + return *this; + } + + // temporary instance has _dtorCalled field set to nullptr + _dtorCalled = std::exchange(other._dtorCalled, nullptr); + + return *this; + } + + private: + bool* _dtorCalled; + }; + + bool dtorCalled = false; + { + LinkedList l; + + l.append({&dtorCalled}); + + ASSERT_EQ(l.size(), 1); + } + + ASSERT_TRUE(dtorCalled); +} + +TEST(LinkedListTest, CannotAllocate) +{ + class null_memory_resource : public pmr::memory_resource + { + private: + void* do_allocate(size_t _Bytes, size_t _Align) override + { + return _value; + } + + void do_deallocate(void* _Ptr, size_t _Bytes, size_t _Align) override + { + } + + bool do_is_equal(const memory_resource& _That) const noexcept override + { + return false; + } + + void* _value = nullptr; + }; + + null_memory_resource mr; + LinkedList l(&mr); + + ASSERT_FALSE(l.append(42)); + + ASSERT_EQ(l.size(), 0); +} + +__declspec(noinline) LinkedList f(){ + LinkedList l; + return l; +} + +TEST(LinkedListTest, MustNotKeepPointerToStack) +{ + LinkedList l; + + ASSERT_TRUE(l.append(42)); + + l = f(); + + ASSERT_EQ(l.size(), 0); + + { + LinkedList ll; + l = std::move(ll); + } + + l.append(21); +} + +__declspec(noinline) void ff(LinkedList& ll) +{ + LinkedList l; + l.swap(ll); +} + +TEST(LinkedListTest, MustNotKeepPointerToStack2) +{ + LinkedList l; + ff(l); + l.append(21); +}