This is an automated email from the ASF dual-hosted git repository. masaori pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/trafficserver.git
The following commit(s) were added to refs/heads/master by this push: new c40271a Reduce unnecesary IOBufferBlock allocation c40271a is described below commit c40271ae7543a87bc90b4278954b79d304bb70b0 Author: Masaori Koshiba <masa...@apache.org> AuthorDate: Tue Sep 3 16:23:00 2019 +0900 Reduce unnecesary IOBufferBlock allocation - Add unit tests for MIOBuffer::write_avail() - Do nothing when the next block of the current writer exists --- iocore/eventsystem/IOBuffer.cc | 13 +- iocore/eventsystem/I_IOBuffer.h | 5 +- iocore/eventsystem/P_IOBuffer.h | 4 +- iocore/eventsystem/unit_tests/test_IOBuffer.cc | 162 +++++++++++++++++++++++++ 4 files changed, 179 insertions(+), 5 deletions(-) diff --git a/iocore/eventsystem/IOBuffer.cc b/iocore/eventsystem/IOBuffer.cc index e1a255a..d2c130b 100644 --- a/iocore/eventsystem/IOBuffer.cc +++ b/iocore/eventsystem/IOBuffer.cc @@ -60,6 +60,9 @@ init_buffer_allocators(int iobuffer_advice) } } +// +// MIOBuffer +// int64_t MIOBuffer::remove_append(IOBufferReader *r) { @@ -173,6 +176,9 @@ MIOBuffer::puts(char *s, int64_t len) return 0; } +// +// IOBufferReader +// int64_t IOBufferReader::read(void *ab, int64_t len) { @@ -262,6 +268,9 @@ IOBufferReader::memcpy(void *ap, int64_t len, int64_t offset) return p; } +// +// IOBufferChain +// int64_t IOBufferChain::write(IOBufferBlock *blocks, int64_t length, int64_t offset) { @@ -349,7 +358,9 @@ IOBufferChain::consume(int64_t size) return zret; } -//-- MIOBufferWriter +// +// MIOBufferWriter +// MIOBufferWriter & MIOBufferWriter::write(const void *data_, size_t length) { diff --git a/iocore/eventsystem/I_IOBuffer.h b/iocore/eventsystem/I_IOBuffer.h index 75a26a9..164ec1a 100644 --- a/iocore/eventsystem/I_IOBuffer.h +++ b/iocore/eventsystem/I_IOBuffer.h @@ -933,9 +933,8 @@ public: void append_block(int64_t asize_index); /** - Adds new block to the end of block list using the block size for - the buffer specified when the buffer was allocated. - + Adds a new block to the end of the block list. Note that this does nothing when the next block of the current writer exists. + The block size is the same as specified size when the buffer was allocated. */ void add_block(); diff --git a/iocore/eventsystem/P_IOBuffer.h b/iocore/eventsystem/P_IOBuffer.h index 6f15d9c..49e6555 100644 --- a/iocore/eventsystem/P_IOBuffer.h +++ b/iocore/eventsystem/P_IOBuffer.h @@ -961,7 +961,9 @@ MIOBuffer::append_block(int64_t asize_index) TS_INLINE void MIOBuffer::add_block() { - append_block(size_index); + if (this->_writer == nullptr || this->_writer->next == nullptr) { + append_block(size_index); + } } TS_INLINE void diff --git a/iocore/eventsystem/unit_tests/test_IOBuffer.cc b/iocore/eventsystem/unit_tests/test_IOBuffer.cc index f380bf0..8128ffb 100644 --- a/iocore/eventsystem/unit_tests/test_IOBuffer.cc +++ b/iocore/eventsystem/unit_tests/test_IOBuffer.cc @@ -173,6 +173,168 @@ TEST_CASE("MIOBuffer", "[iocore]") free_MIOBuffer(miob); } + + SECTION("write_avail") + { + MIOBuffer *miob = new_MIOBuffer(); + IOBufferReader *miob_r = miob->alloc_reader(); + uint8_t buf[8192]; + memset(buf, 0xAA, sizeof(buf)); + + // initial state + CHECK(miob->block_size() == 4096); + CHECK(miob->current_write_avail() == 4096); + CHECK(miob->write_avail() == 4096); + + SECTION("water_mark == 0 (default)") + { + REQUIRE(miob->water_mark == 0); + + // fill half of the current buffer + miob->write(buf, 2048); + CHECK(miob->max_read_avail() == 2048); + CHECK(miob->current_write_avail() == 2048); + CHECK(miob->high_water() == true); + CHECK(miob->current_low_water() == false); + CHECK(miob->write_avail() == 2048); ///< should have no side effect + + // fill all of the current buffer + miob->write(buf, 2048); + CHECK(miob->max_read_avail() == 4096); + CHECK(miob->current_write_avail() == 0); + CHECK(miob->high_water() == true); + CHECK(miob->current_low_water() == true); + CHECK(miob->write_avail() == 0); ///< should have no side effect + + // consume half of the data + miob_r->consume(2048); + CHECK(miob->max_read_avail() == 2048); + CHECK(miob->current_write_avail() == 0); + CHECK(miob->high_water() == true); + CHECK(miob->current_low_water() == true); + CHECK(miob->write_avail() == 0); ///< should have no side effect + + // consume all of the data + miob_r->consume(2048); + CHECK(miob->max_read_avail() == 0); + CHECK(miob->current_write_avail() == 0); + CHECK(miob->high_water() == false); + CHECK(miob->current_low_water() == true); + CHECK(miob->write_avail() == 4096); ///< should have a side effect: add a new block + + CHECK(miob->max_read_avail() == 0); + CHECK(miob->current_write_avail() == 4096); + CHECK(miob->high_water() == false); + CHECK(miob->current_low_water() == false); + CHECK(miob->write_avail() == 4096); ///< should have no side effect + } + + SECTION("water_mark == half of block size") + { + miob->water_mark = 2048; + REQUIRE(miob->water_mark * 2 == miob->block_size()); + + // fill half of the current buffer + miob->write(buf, 2048); + CHECK(miob->max_read_avail() == 2048); + CHECK(miob->current_write_avail() == 2048); + CHECK(miob->high_water() == false); + CHECK(miob->current_low_water() == true); + CHECK(miob->write_avail() == 6144); ///< should have a side effect: add a new block + + CHECK(miob->max_read_avail() == 2048); + CHECK(miob->current_write_avail() == 6144); + CHECK(miob->high_water() == false); + CHECK(miob->current_low_water() == false); + CHECK(miob->write_avail() == 6144); ///< should have no side effect + + // fill all of the current buffer + miob->write(buf, 6144); + CHECK(miob->max_read_avail() == 8192); + CHECK(miob->current_write_avail() == 0); + CHECK(miob->high_water() == true); + CHECK(miob->current_low_water() == true); + CHECK(miob->write_avail() == 0); ///< should have no side effect + + // consume half of the data + miob_r->consume(4096); + CHECK(miob->max_read_avail() == 4096); + CHECK(miob->current_write_avail() == 0); + CHECK(miob->high_water() == true); + CHECK(miob->current_low_water() == true); + CHECK(miob->write_avail() == 0); ///< should have no side effect + + // consume all of the data + miob_r->consume(4096); + CHECK(miob->max_read_avail() == 0); + CHECK(miob->current_write_avail() == 0); + CHECK(miob->high_water() == false); + CHECK(miob->current_low_water() == true); + CHECK(miob->write_avail() == 4096); ///< should have a side effect: add a new block + + CHECK(miob->max_read_avail() == 0); + CHECK(miob->current_write_avail() == 4096); + CHECK(miob->high_water() == false); + CHECK(miob->current_low_water() == false); + CHECK(miob->write_avail() == 4096); ///< should have no side effect + } + + SECTION("water_mark == block_size()") + { + miob->water_mark = 4096; + REQUIRE(miob->water_mark == miob->block_size()); + + // fill half of the current buffer + miob->write(buf, 2048); + CHECK(miob->max_read_avail() == 2048); + CHECK(miob->current_write_avail() == 2048); + CHECK(miob->high_water() == false); + CHECK(miob->current_low_water() == true); + CHECK(miob->write_avail() == 6144); ///< should have a side effect: add a new block + + CHECK(miob->max_read_avail() == 2048); + CHECK(miob->current_write_avail() == 6144); + CHECK(miob->high_water() == false); + CHECK(miob->current_low_water() == false); + CHECK(miob->write_avail() == 6144); ///< should have no side effect + + // fill all of the current buffer + miob->write(buf, 6144); + CHECK(miob->max_read_avail() == 8192); + CHECK(miob->current_write_avail() == 0); + CHECK(miob->high_water() == true); + CHECK(miob->current_low_water() == true); + CHECK(miob->write_avail() == 0); ///< should have no side effect + + // consume half of the data + miob_r->consume(4096); + CHECK(miob->max_read_avail() == 4096); + CHECK(miob->current_write_avail() == 0); + CHECK(miob->high_water() == false); + CHECK(miob->current_low_water() == true); + CHECK(miob->write_avail() == 4096); ///< should have a side effect: add a new block + IOBufferBlock *tail = miob->_writer->next.get(); + CHECK(tail != nullptr); + + CHECK(miob->max_read_avail() == 4096); + CHECK(miob->current_write_avail() == 4096); + CHECK(miob->high_water() == false); + CHECK(miob->current_low_water() == true); + CHECK(miob->write_avail() == 4096); ///< should have no side effect + CHECK(tail == miob->_writer->next.get()); ///< the tail block should not be changed + + // consume all of the data + miob_r->consume(4096); + CHECK(miob->max_read_avail() == 0); + CHECK(miob->current_write_avail() == 4096); + CHECK(miob->high_water() == false); + CHECK(miob->current_low_water() == true); + CHECK(miob->write_avail() == 4096); ///< should have no side effect + CHECK(tail == miob->_writer->next.get()); ///< the tail block should not be changed + } + + free_MIOBuffer(miob); + } } struct EventProcessorListener : Catch::TestEventListenerBase {