RFR(xs): 8146905 - cleanup ostream, staticBufferStream

[David Holmes]

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.

src/share/vm/utilities/ostream.hpp

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?

Thanks,
David
----


> 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
>>>>
>>