ORC-1280:[C++] Implement block-based buffer(Part I) #1271

coderex2522 · 2022-10-06T13:57:25Z

What changes were proposed in this pull request?

This patch implements a block-based buffer allocator. Provides smarter memory management for the buffer of BufferedOutputStream class.

Why are the changes needed?

This patch implements smarter memory management, which can effectively solve the issue-1240. And replacing DataBuffer with BlockBuffer in BufferedOutputStream class will be implemented in another patch.

How was this patch tested?

The UTs in TestBlockBuffer.cc can test this patch.

…putStream

dongjoon-hyun · 2022-10-06T16:49:43Z

c++/src/MemoryPool.hh

+#ifndef ORC_MEMORYPOOL_IMPL_HH
+#define ORC_MEMORYPOOL_IMPL_HH
+
+#include "orc/MemoryPool.hh"


It's a little confusing making another MemoryPool.hh like this. Could you create a new file instead of MemoryPool.hh file?

Sure, I rename src/MemoryPool.hh into src/BlockBuffer.hh.

dongjoon-hyun · 2022-10-06T16:50:45Z

Thank you for making a PR. I have one comment about file structure, #1271 (comment).

cc @wgtmac , @stiga-huang

dongjoon-hyun · 2022-10-07T06:46:33Z

c++/src/BlockBuffer.hh

@@ -0,0 +1,90 @@
+


Redundant space.

dongjoon-hyun · 2022-10-07T06:46:59Z

c++/test/TestBlockBuffer.cc

@@ -0,0 +1,82 @@
+


Redundant space.

dongjoon-hyun · 2022-10-07T06:52:02Z

It seems that the GitHub Action labeler is broken. Let me fix it.

wgtmac

Thanks @coderex2522 for the improvement! Would be great to equip the new class with more comment in detail.

wgtmac · 2022-10-07T14:46:51Z

c++/src/BlockBuffer.hh

+ Block() : data(nullptr), size(0) {}
+ Block(char* _data, uint64_t _size) : data(_data), size(_size) {}
+ Block(const Block& block) = default;
+ ~Block() {}


~Block() = default;

wgtmac · 2022-10-07T14:47:45Z

c++/src/BlockBuffer.hh

+ void resize(uint64_t size);
+ void reserve(uint64_t capacity);
+ };
+} // namespace


// namespace orc

wgtmac · 2022-10-07T14:54:29Z

c++/src/BlockBuffer.hh

+ /**
+ * Get the Block object
+ */
+ Block getBlock(uint64_t blockIndex);


We need some detail explanation with regard to the mechanism and behavior of the BlockBuffer. Especially concepts like block, block index and block number.

Added some description for block, blockbuffer, block index and block number.

wgtmac · 2022-10-07T14:55:06Z

c++/src/BlockBuffer.hh

+ /**
+ * Get the Block object
+ */
+ Block getBlock(uint64_t blockIndex);


Should we provide mutable and immutable overloads?

Because the getBlock function does not modify class member variables, the const attribute is added to the function.

wgtmac · 2022-10-08T05:04:30Z

c++/src/BlockBuffer.hh

+ }
+
+ void resize(uint64_t size);
+ void reserve(uint64_t capacity);


Insert a new line before reserve function. Better to explain what will happen in the reserve function

Added some comments to explain the reserve function.

wgtmac · 2022-10-08T05:05:11Z

c++/src/BlockBuffer.hh

+ return currentCapacity;
+ }
+
+ void resize(uint64_t size);


Should we support shrinkToFit parameter?

Class DataBuffer only recycles memory during destructor, so class BlockBuffer does not support shrink function, which can be further optimized later.

wgtmac · 2022-10-08T05:06:29Z

c++/src/BlockBuffer.hh

+ * Block points to a section of memory allocated by BlockBuffer,
+ * containing the corresponding physical memory address and size.
+ */
+ struct Block {


Is it better to make it nested class of BlockBuffer?

As it stands struct Block only serves class BlockBuffer, so it makes reasonable to treat it as a nested class of class BlockBuffer.

wgtmac · 2022-10-08T05:10:06Z

c++/src/BlockBuffer.hh

+ /**
+ * Get a empty block or a new block if the buffer is exhausted.
+ * If the last allocated block size is less than blockSize,
+ * the empty block size is blockSize - lastBlockSize.


Where is the lastBlockSize?
It would be more user-friendly to get the available size of the returned block via output parameter.

lastBlockSize means the size of the last allocated block. The comment has been modified here.
The returned block is used to describe the section of memory that can be read or written. And I add comment to the size in struct Block. So I don't recommend adding extra output parameter.

wgtmac · 2022-10-08T05:11:19Z

c++/src/BlockBuffer.hh

+ * Otherwise, the empty block size is blockSize.
+ * @return a empty block object
+ */
+ Block getEmptyBlock();


rename to getNextBlock or getBlockToWrite?

Done. Rename to getNextBlock.

c++/src/BlockBuffer.hh

wgtmac · 2022-10-08T05:14:10Z

c++/src/BlockBuffer.hh

+ Block getEmptyBlock();
+
+ /**
+ * Get the number of allocated blocks


Get the number of blocks that are fully or partially occupied

wgtmac

I have left an inline comment. Overall looks good.

wgtmac · 2022-10-09T05:00:50Z

c++/src/BlockBuffer.cc

+ Block emptyBlock(
+ blocks[currentSize / blockSize] + currentSize % blockSize,
+ blockSize - currentSize % blockSize);
+ currentSize = (currentSize / blockSize + 1) * blockSize;


Should we provide a function to update currentSize to reflect the actual size written? Maybe something setSize() or backup(). This looks weird when the returned block is written partially meaning that currentSize is larger than used

To provide better usability, we may provide a function like void append(const char data, size_t size)* to append data to the buffer and manage the blocks internally. This can be a separate patch.

If user only uses part of the block, the size of BlockBuffer can currently be set via the resize() function. BlockBuffer class is temporarily used to replace DataBuffer in BufferedOutputStream, and the existing functions conform to BufferedOutputStream's usage behavior.

The append function can be added additionally later if there are usage scenarios.

Since the flush() function of class BufferedOutputStream needs to access all allocated memory blocks, it needs class BlockBuffer to provide an interface in order for BufferedOutputStream to access all allocated memory blocks.

wgtmac

+1 LGTM

dongjoon-hyun

+1, LGTM, too.

wgtmac · 2022-10-12T01:42:21Z

I've submitted and closed it. Thanks @coderex2522 and @dongjoon-hyun

This closes apache#1271

coderex2522 added 4 commits October 2, 2022 17:26

ORC-1280: [C++] Implement optimized buffer management for BufferedOut…

59081e7

…putStream

ORC-1280: [C++] move BlockBuffer implementation into src folder

cbec2d2

ORC-1280: [C++] add getEmptyBlock implementation

cecb4b2

ORC-1280: [C++] complement the UT of block buffer

3ac5489

coderex2522 changed the title ~~ORC-1280:[C++][Part I] Implement block-based buffer~~ ORC-1280:[C++] Implement block-based buffer(Part I) Oct 6, 2022

ORC-1280: [C++] fix the format

5e8c768

dongjoon-hyun reviewed Oct 6, 2022

View reviewed changes

dongjoon-hyun added this to the 1.9.0 milestone Oct 6, 2022

ORC-1280: [C++] rename src/MemoryPool.hh into src/BlockBuffer.hh

9733151

dongjoon-hyun reviewed Oct 7, 2022

View reviewed changes

ORC-1280: [C++] fix the file format

0aad043

github-actions bot added the CPP label Oct 7, 2022

wgtmac reviewed Oct 7, 2022

View reviewed changes

coderex2522 added 2 commits October 8, 2022 11:39

ORC-1280: [C++] add some comment for Block and BlockBuffer

937a6d1

ORC-1280: [C++] fix some comments

a9789eb

wgtmac reviewed Oct 8, 2022

View reviewed changes

coderex2522 added 2 commits October 8, 2022 14:30

ORC-1280: [C++] make Block nested class of BlockBuffer

3020b0f

ORC-1280: [C++] fix the header macro

8e4f148

wgtmac reviewed Oct 9, 2022

View reviewed changes

ORC-1280: [C++] modify the comment about reserve function

6ee8ac2

wgtmac approved these changes Oct 10, 2022

View reviewed changes

dongjoon-hyun approved these changes Oct 11, 2022

View reviewed changes

wgtmac merged commit 8cf1047 into apache:main Oct 12, 2022

cxzl25 pushed a commit to cxzl25/orc that referenced this pull request Jan 11, 2024

ORC-1280: [C++] Implement block-based buffer(Part I)

ead212b

This closes apache#1271

ORC-1280:[C++] Implement block-based buffer(Part I) #1271

ORC-1280:[C++] Implement block-based buffer(Part I) #1271

Conversation

coderex2522 commented Oct 6, 2022

What changes were proposed in this pull request?

Why are the changes needed?

How was this patch tested?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dongjoon-hyun commented Oct 6, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dongjoon-hyun commented Oct 7, 2022 • edited Loading

wgtmac left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coderex2522 Oct 8, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coderex2522 Oct 9, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wgtmac left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wgtmac left a comment

Choose a reason for hiding this comment

dongjoon-hyun left a comment

Choose a reason for hiding this comment

wgtmac commented Oct 12, 2022

dongjoon-hyun commented Oct 6, 2022 •

edited

Loading

dongjoon-hyun commented Oct 7, 2022 •

edited

Loading

coderex2522 Oct 8, 2022 •

edited

Loading

coderex2522 Oct 9, 2022 •

edited

Loading