You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@trafficserver.apache.org by zw...@apache.org on 2020/04/21 15:26:02 UTC
[trafficserver] 03/05: Reduce unnecesary IOBufferBlock allocation
This is an automated email from the ASF dual-hosted git repository.
zwoop pushed a commit to branch 8.1.x
in repository https://gitbox.apache.org/repos/asf/trafficserver.git
commit 3e00a0a4dd9a3994bfe4284062e2362af61a4e2c
Author: Masaori Koshiba <ma...@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
(cherry picked from commit c40271ae7543a87bc90b4278954b79d304bb70b0)
Conflicts:
iocore/eventsystem/IOBuffer.cc
---
iocore/eventsystem/IOBuffer.cc | 10 +-
iocore/eventsystem/I_IOBuffer.h | 5 +-
iocore/eventsystem/P_IOBuffer.h | 4 +-
iocore/eventsystem/unit_tests/test_IOBuffer.cc | 162 +++++++++++++++++++++++++
4 files changed, 176 insertions(+), 5 deletions(-)
diff --git a/iocore/eventsystem/IOBuffer.cc b/iocore/eventsystem/IOBuffer.cc
index d5f5207..a45548f 100644
--- a/iocore/eventsystem/IOBuffer.cc
+++ b/iocore/eventsystem/IOBuffer.cc
@@ -62,6 +62,9 @@ init_buffer_allocators(int iobuffer_advice)
}
}
+//
+// MIOBuffer
+//
int64_t
MIOBuffer::remove_append(IOBufferReader *r)
{
@@ -190,6 +193,9 @@ MIOBuffer::puts(char *s, int64_t len)
return 0;
}
+//
+// IOBufferReader
+//
int64_t
IOBufferReader::read(void *ab, int64_t len)
{
@@ -279,7 +285,9 @@ IOBufferReader::memcpy(const void *ap, int64_t len, int64_t offset)
return p;
}
-//-- 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 852ccee..c249e46 100644
--- a/iocore/eventsystem/I_IOBuffer.h
+++ b/iocore/eventsystem/I_IOBuffer.h
@@ -839,9 +839,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 0820859..05457d2 100644
--- a/iocore/eventsystem/P_IOBuffer.h
+++ b/iocore/eventsystem/P_IOBuffer.h
@@ -987,7 +987,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 c83f68a..9868889 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 {