8260030: Improve stringStream buffer handling

Reviewed-by: iklam, kbarrett

src/hotspot/share/utilities/ostream.cpp

@@ -310,46 +310,72 @@ void outputStream::print_data(void* data, size_t len, bool with_ascii) {
   }
 }
 
-stringStream::stringStream(size_t initial_size) : outputStream() {
-  buffer_length = initial_size;
-  buffer        = NEW_C_HEAP_ARRAY(char, buffer_length, mtInternal);
-  buffer_pos    = 0;
-  buffer_fixed  = false;
+stringStream::stringStream(size_t initial_capacity) :
+  outputStream(),
+  _buffer(_small_buffer),
+  _written(0),
+  _capacity(sizeof(_small_buffer)),
+  _is_fixed(false)
+{
+  if (initial_capacity > _capacity) {
+    grow(initial_capacity);
+  }
   zero_terminate();
 }
 
 // useful for output to fixed chunks of memory, such as performance counters
-stringStream::stringStream(char* fixed_buffer, size_t fixed_buffer_size) : outputStream() {
-  buffer_length = fixed_buffer_size;
-  buffer        = fixed_buffer;
-  buffer_pos    = 0;
-  buffer_fixed  = true;
+stringStream::stringStream(char* fixed_buffer, size_t fixed_buffer_size) :
+  outputStream(),
+  _buffer(fixed_buffer),
+  _written(0),
+  _capacity(fixed_buffer_size),
+  _is_fixed(true)
+{
   zero_terminate();
 }
 
+// Grow backing buffer to desired capacity. Don't call for fixed buffers
+void stringStream::grow(size_t new_capacity) {
+  assert(!_is_fixed, "Don't call for caller provided buffers");
+  assert(new_capacity > _capacity, "Sanity");
+  assert(new_capacity > sizeof(_small_buffer), "Sanity");
+  if (_buffer == _small_buffer) {
+    _buffer = NEW_C_HEAP_ARRAY(char, new_capacity, mtInternal);
+    _capacity = new_capacity;
+    if (_written > 0) {
+      ::memcpy(_buffer, _small_buffer, _written);
+    }
+    zero_terminate();
+  } else {
+    _buffer = REALLOC_C_HEAP_ARRAY(char, _buffer, new_capacity, mtInternal);
+    _capacity = new_capacity;
+  }
+}
+
 void stringStream::write(const char* s, size_t len) {
-  size_t write_len = len;               // number of non-null bytes to write
-  size_t end = buffer_pos + len + 1;    // position after write and final '\0'
-  if (end > buffer_length) {
-    if (buffer_fixed) {
-      // if buffer cannot resize, silently truncate
-      end = buffer_length;
-      write_len = end - buffer_pos - 1; // leave room for the final '\0'
-    } else {
-      // For small overruns, double the buffer.  For larger ones,
-      // increase to the requested size.
-      if (end < buffer_length * 2) {
-        end = buffer_length * 2;
-      }
-      buffer = REALLOC_C_HEAP_ARRAY(char, buffer, end, mtInternal);
-      buffer_length = end;
+  assert(_capacity >= _written + 1, "Sanity");
+  if (len == 0) {
+    return;
+  }
+  const size_t reasonable_max_len = 1 * G;
+  if (len >= reasonable_max_len) {
+    assert(false, "bad length? (" SIZE_FORMAT ")", len);
+    return;
+  }
+  size_t write_len = 0;
+  if (_is_fixed) {
+    write_len = MIN2(len, _capacity - _written - 1);
+  } else {
+    write_len = len;
+    size_t needed = _written + len + 1;
+    if (needed > _capacity) {
+      grow(MAX2(needed, _capacity * 2));
     }
   }
-  // invariant: buffer is always null-terminated
-  guarantee(buffer_pos + write_len + 1 <= buffer_length, "stringStream oob");
+  assert(_written + write_len + 1 <= _capacity, "stringStream oob");
   if (write_len > 0) {
-    memcpy(buffer + buffer_pos, s, write_len);
-    buffer_pos += write_len;
+    ::memcpy(_buffer + _written, s, write_len);
+    _written += write_len;
     zero_terminate();
   }
 
@@ -360,21 +386,22 @@ void stringStream::write(const char* s, size_t len) {
 }
 
 void stringStream::zero_terminate() {
-  assert(buffer != NULL &&
-         buffer_pos < buffer_length, "sanity");
-  buffer[buffer_pos] = '\0';
+  assert(_buffer != NULL &&
+         _written < _capacity, "sanity");
+  _buffer[_written] = '\0';
 }
 
 void stringStream::reset() {
-  buffer_pos = 0; _precount = 0; _position = 0;
+  _written = 0; _precount = 0; _position = 0;
+  _newlines = 0;
   zero_terminate();
 }
 
 char* stringStream::as_string(bool c_heap) const {
   char* copy = c_heap ?
-    NEW_C_HEAP_ARRAY(char, buffer_pos + 1, mtInternal) : NEW_RESOURCE_ARRAY(char, buffer_pos + 1);
-  strncpy(copy, buffer, buffer_pos);
-  copy[buffer_pos] = 0;  // terminating null
+    NEW_C_HEAP_ARRAY(char, _written + 1, mtInternal) : NEW_RESOURCE_ARRAY(char, _written + 1);
+  ::memcpy(copy, _buffer, _written);
+  copy[_written] = 0;  // terminating null
   if (c_heap) {
     // Need to ensure our content is written to memory before we return
     // the pointer to it.
@@ -384,8 +411,8 @@ char* stringStream::as_string(bool c_heap) const {
 }
 
 stringStream::~stringStream() {
-  if (buffer_fixed == false && buffer != NULL) {
-    FREE_C_HEAP_ARRAY(char, buffer);
+  if (!_is_fixed && _buffer != _small_buffer) {
+    FREE_C_HEAP_ARRAY(char, _buffer);
   }
 }
 

src/hotspot/share/utilities/ostream.hpp

@@ -87,7 +87,6 @@ class outputStream : public ResourceObj {
    // sizing
    int width()    const { return _width;    }
    int position() const { return _position; }
-   int newlines() const { return _newlines; }
    julong count() const { return _precount + _position; }
    void set_count(julong count) { _precount = count - _position; }
    void set_position(int pos)   { _position = pos; }
@@ -192,28 +191,31 @@ class ttyUnlocker: StackObj {
 // for writing to strings; buffer will expand automatically.
 // Buffer will always be zero-terminated.
 class stringStream : public outputStream {
- protected:
-  char*  buffer;
-  size_t buffer_pos;
-  size_t buffer_length;
-  bool   buffer_fixed;
+  char*  _buffer;
+  size_t _written;  // Number of characters written, excluding termin. zero
+  size_t _capacity;
+  const bool _is_fixed;
+  char   _small_buffer[48];
+
+  // Grow backing buffer to desired capacity.
+  void grow(size_t new_capacity);
 
   // zero terminate at buffer_pos.
   void zero_terminate();
 
  public:
   // Create a stringStream using an internal buffer of initially initial_bufsize size;
   // will be enlarged on demand. There is no maximum cap.
-  stringStream(size_t initial_bufsize = 256);
+  stringStream(size_t initial_capacity = 0);
   // Creates a stringStream using a caller-provided buffer. Will truncate silently if
   // it overflows.
   stringStream(char* fixed_buffer, size_t fixed_buffer_size);
   ~stringStream();
   virtual void write(const char* c, size_t len);
   // Return number of characters written into buffer, excluding terminating zero and
   // subject to truncation in static buffer mode.
-  size_t      size() const { return buffer_pos; }
-  const char* base() const { return buffer; }
+  size_t      size() const { return _written; }
+  const char* base() const { return _buffer; }
   void  reset();
   // copy to a resource, or C-heap, array as requested
   char* as_string(bool c_heap = false) const;

test/hotspot/gtest/utilities/test_ostream.cpp

@@ -31,20 +31,17 @@
 
 #include "unittest.hpp"
 
-static size_t print_lorem(outputStream* st, bool short_len) {
+static size_t print_lorem(outputStream* st) {
   // Create a ResourceMark just to make sure the stream does not use ResourceArea
   ResourceMark rm;
   static const char* const lorem = "Lorem ipsum dolor sit amet, consectetur adipiscing elit, "
       "sed do eiusmod tempor incididunt ut labore et dolore magna aliqua. Lacinia at quis "
       "risus sed vulputate odio ut enim blandit. Amet risus nullam eget felis eget. Viverra "
       "orci sagittis eu volutpat odio facilisis mauris sit. Erat velit scelerisque in dictum non.";
   static const size_t len_lorem = strlen(lorem);
-  size_t len;
-  if (short_len) {
-    len = os::random() % 10;
-  } else {
-    len = MAX2(1, (int)(os::random() % len_lorem));
-  }
+  // Randomly alternate between short and long writes at a ratio of 9:1.
+  const bool short_write = (os::random() % 10) > 0;
+  const size_t len = os::random() % (short_write ? 10 : len_lorem);
   st->write(lorem, len);
   return len;
 }
@@ -53,12 +50,11 @@ static void test_stringStream_is_zero_terminated(const stringStream* ss) {
   ASSERT_EQ(ss->base()[ss->size()], '\0');
 }
 
-
-static void do_test_stringStream(stringStream* ss, bool short_len, size_t expected_cap) {
+static void do_test_stringStream(stringStream* ss, size_t expected_cap) {
   test_stringStream_is_zero_terminated(ss);
   size_t written = 0;
   for (int i = 0; i < 1000; i ++) {
-    written += print_lorem(ss, short_len);
+    written += print_lorem(ss);
     if (expected_cap > 0 && written >= expected_cap) {
       ASSERT_EQ(ss->size(), expected_cap - 1);
     } else {
@@ -73,14 +69,18 @@ static void do_test_stringStream(stringStream* ss, bool short_len, size_t expect
   test_stringStream_is_zero_terminated(ss);
 }
 
-TEST_VM(ostream, stringStream_dynamic_realloc_1) {
-  stringStream ss(2); // dynamic buffer with very small starting size
-  do_test_stringStream(&ss, false, 0);
+TEST_VM(ostream, stringStream_dynamic_start_with_internal_buffer) {
+  stringStream ss;
+  do_test_stringStream(&ss, 0);
+  ss.reset();
+  do_test_stringStream(&ss, 0);
 }
 
-TEST_VM(ostream, stringStream_dynamic_realloc_2) {
-  stringStream ss(2); // dynamic buffer with very small starting size
-  do_test_stringStream(&ss, true, 0);
+TEST_VM(ostream, stringStream_dynamic_start_with_malloced_buffer) {
+  stringStream ss(128);
+  do_test_stringStream(&ss, 0);
+  ss.reset();
+  do_test_stringStream(&ss, 0);
 }
 
 TEST_VM(ostream, stringStream_static) {
@@ -89,7 +89,7 @@ TEST_VM(ostream, stringStream_static) {
   *canary_at = 'X';
   size_t stream_buf_size = sizeof(buffer) - 1;
   stringStream ss(buffer, stream_buf_size);
-  do_test_stringStream(&ss, false, stream_buf_size);
+  do_test_stringStream(&ss, stream_buf_size);
   ASSERT_EQ(*canary_at, 'X'); // canary
 }
 
@@ -101,7 +101,7 @@ TEST_VM(ostream, bufferedStream_static) {
   bufferedStream bs(buf, stream_buf_size);
   size_t written = 0;
   for (int i = 0; i < 100; i ++) {
-    written += print_lorem(&bs, true);
+    written += print_lorem(&bs);
     if (written < stream_buf_size) {
       ASSERT_EQ(bs.size(), written);
     } else {
@@ -116,7 +116,7 @@ TEST_VM(ostream, bufferedStream_dynamic_small) {
   size_t written = 0;
   // The max cap imposed is 100M, we should be safely below this in this test.
   for (int i = 0; i < 10; i ++) {
-    written += print_lorem(&bs, true);
+    written += print_lorem(&bs);
     ASSERT_EQ(bs.size(), written);
   }
 }
@@ -130,7 +130,7 @@ TEST_VM(ostream, bufferedStream_dynamic_large) {
   // Note that this will assert in debug builds which is the expected behavior.
   size_t expected_cap_at = 100 * M;
   for (int i = 0; i < 10000000; i ++) {
-    written += print_lorem(&bs, false);
+    written += print_lorem(&bs);
     if (written < expected_cap_at) {
       ASSERT_EQ(bs.size(), written);
     } else {