RFR(xs): 8146905 - cleanup ostream, staticBufferStream

David Holmes david.holmes at oracle.com
Wed Jan 27 05:27:02 UTC 2016

Hi Thomas,

On 20/01/2016 7:29 PM, Thomas Stüfe wrote:
> Thank you Volker!
> Still need a second reviewer, and once hs-rt is open again I'll need a
> sponsor too.

Sorry for the delay ... I can sponsor this for you.

Overall looks okay - took me a little while to wrap my head around it.


Typo:  // Caller may specify an own scratch buffer

an -> their


Please update copyright years.


What testing have you done for this? Is it sufficient to generate some 
crash logs and validate the content is the same before and after this fix?


> Kind Regards, Thomas
> On Wed, Jan 20, 2016 at 9:32 AM, Volker Simonis <volker.simonis at gmail.com>
> wrote:
>> Hi Thomas,
>> thanks for doing this nice cleanup.
>> The change looks good. Can you please just adapt the comment at line
>> 987 and remove the reference to staticBufferStream from there as well.
>> Regards,
>> Volker
>> On Wed, Jan 20, 2016 at 8:54 AM, Thomas Stüfe <thomas.stuefe at gmail.com>
>> wrote:
>>> May I please have a review for this? Thanks!
>>> On Wed, Jan 13, 2016 at 3:00 PM, Thomas Stüfe <thomas.stuefe at gmail.com>
>>> wrote:
>>>> Dear all,
>>>> please take a look at this small cleanup of ostream.
>>>> bug report: https://bugs.openjdk.java.net/browse/JDK-8146905
>>>> webrev:
>> http://cr.openjdk.java.net/~stuefe/webrevs/8146905-get-rid-of-staticBufferStream/webrev.00/webrev/
>>>> Basically, it gets rid of the staticBufferStream class by moving its one
>>>> feature up to the parent outputStream class:
>>>> When printing the error log in vmError.cpp, we want to use as little
>> stack
>>>> space as possible. outputStream class uses on-stack buffers to assemble
>>>> snprintf functions. So, staticBufferStream was introduced as a child of
>>>> outputStream which overwrites the print functions and makes them use a
>>>> caller provided buffer. It then delegates the real writing to a
>> connected
>>>> stream object.
>>>> The problem with that approach is that this is not a good fit for a
>> child
>>>> class.Not all operations possible with outputStream are cleanly
>> delegated
>>>> to the connected stream class, so a staticBufferStream behaves sometimes
>>>> differently from all other streams (see e.g. JDK-8145410, which will be
>>>> automatically fixed with this change too).
>>>> Another problem is that this delegation model leads to some code
>>>> duplication, because all print() methods of outputStream are practically
>>>> duplicated in staticBufferStream.
>>>> Another cosmetic note is that all other child classes of outputStream
>>>> (bufferedStream, stringStream, fileStream...) are specializations in
>> term
>>>> of log destination, not internal behaviour.
>>>> Note that I implemented this in a very simple way without using
>> templates,
>>>> because I wanted to keep the changes as small as possible.
>>>> Kind Regards, Thomas

More information about the hotspot-runtime-dev mailing list