Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Allow byte_stream->byte_slice conversion to shrink unused buffer space #7005

Merged
merged 1 commit into from
Mar 20, 2021
Merged

Allow byte_stream->byte_slice conversion to shrink unused buffer space #7005

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion contrib/epee/include/byte_slice.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,7 @@ namespace epee
explicit byte_slice(std::string&& buffer);

//! Convert `stream` into a slice with zero allocations.
explicit byte_slice(byte_stream&& stream) noexcept;
explicit byte_slice(byte_stream&& stream, bool shrink = true);

byte_slice(byte_slice&& source) noexcept;
~byte_slice() noexcept = default;
Expand Down
24 changes: 20 additions & 4 deletions contrib/epee/src/byte_slice.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,11 @@
#include "byte_slice.h"
#include "byte_stream.h"

namespace
{
const std::size_t page_size = 4096;
}

namespace epee
{
struct byte_slice_data
Expand Down Expand Up @@ -173,16 +178,27 @@ namespace epee
: byte_slice(adapt_buffer{}, std::move(buffer))
{}

byte_slice::byte_slice(byte_stream&& stream) noexcept
byte_slice::byte_slice(byte_stream&& stream, const bool shrink)
: storage_(nullptr), portion_(stream.data(), stream.size())
{
if (stream.size())
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

From cecca7a0fe43044e8f163a748347868f38b1b5e6 Mon Sep 17 00:00:00 2001
From: anon <anon [at] nowhere>
Date: Sat, 20 Feb 2021 19:56:02 +0000
Subject: [PATCH] byte_slice: simplify

---
 contrib/epee/src/byte_slice.cpp | 22 +++++++++-------------
 1 file changed, 9 insertions(+), 13 deletions(-)

diff --git a/contrib/epee/src/byte_slice.cpp b/contrib/epee/src/byte_slice.cpp
index 453b63a4c..2e8a2f820 100644
--- a/contrib/epee/src/byte_slice.cpp
+++ b/contrib/epee/src/byte_slice.cpp
@@ -181,24 +181,20 @@ namespace epee
   byte_slice::byte_slice(byte_stream&& stream, const bool shrink)
     : storage_(nullptr), portion_(stream.data(), stream.size())
   {
-    if (portion_.size())
+    if (stream.size())
     {
-      byte_buffer buf;
       if (shrink && page_size <= stream.available())
       {
-          buf = byte_buffer_resize(stream.take_buffer(), portion_.size());
-          if (!buf)
-            throw std::bad_alloc{};
-          portion_ = {buf.get(), portion_.size()};
+        auto buf = byte_buffer_resize(stream.take_buffer(), portion_.size());
+        if (!buf)
+          throw std::bad_alloc{};
+        portion_ = {buf.get(), portion_.size()};
+        storage_.reset(new (buf.release() - sizeof(raw_byte_slice)) raw_byte_slice{});
       }
-      else // no need to shrink buffer
-        buf = stream.take_buffer();
-
-      std::uint8_t* const data = buf.release() - sizeof(raw_byte_slice);
-      new (data) raw_byte_slice{};
-      storage_.reset(reinterpret_cast<raw_byte_slice*>(data));
+      else
+        storage_.reset(new (stream.take_buffer().release() - sizeof(raw_byte_slice)) raw_byte_slice{});
     }
-    else // empty stream
+    else
       portion_ = nullptr;
   }
 
-- 
2.30.1

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How is this an improvement? It ends up duplicating the placement new call with no obvious benefit.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

The above patch can be ignored if there is no obvious benefit.

if (portion_.size())
{
std::uint8_t* const data = stream.take_buffer().release() - sizeof(raw_byte_slice);
byte_buffer buf;
if (shrink && page_size <= stream.available())
{
buf = byte_buffer_resize(stream.take_buffer(), portion_.size());
if (!buf)
throw std::bad_alloc{};
portion_ = {buf.get(), portion_.size()};
}
else // no need to shrink buffer
buf = stream.take_buffer();

std::uint8_t* const data = buf.release() - sizeof(raw_byte_slice);
new (data) raw_byte_slice{};
storage_.reset(reinterpret_cast<raw_byte_slice*>(data));
}
else
else // empty stream
portion_ = nullptr;
}

Expand Down
4 changes: 3 additions & 1 deletion tests/unit_tests/epee_utils.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1090,11 +1090,13 @@ TEST(ByteStream, ToByteSlice)

epee::byte_stream stream;

stream.reserve(128*1024);
stream.write(source);
EXPECT_EQ(sizeof(source), stream.size());
EXPECT_EQ(128*1024, stream.capacity());
EXPECT_TRUE(equal(source, byte_span{stream.data(), stream.size()}));

const epee::byte_slice slice{std::move(stream)};
const epee::byte_slice slice{std::move(stream), true};
EXPECT_EQ(0u, stream.size());
EXPECT_EQ(0u, stream.available());
EXPECT_EQ(0u, stream.capacity());
Expand Down