RFR(xs): 8146905 - cleanup ostream, staticBufferStream

[Thomas Stüfe]

Hi David,

See here the new webrev.

http://cr.openjdk.java.net/~stuefe/webrevs/8146905-get-rid-of-staticBufferStream/webrev.02/webrev

I changed the comment for set_scratch_buffer() and the copyright years, as
per your request, and left the rest unchanged.

On Wed, Jan 27, 2016 at 6:27 AM, David Holmes <david.holmes at oracle.com>
wrote:

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

For Linux x64, windows and aix I tested manually. hs-err files look ok,
finally on AIX too. On Linux I also diffed an old with the new hs-err file,
to see if there is anything different, but could not spot anything apart
the expected differences for two different runs of the error handler.

I also ran jtreg test for hotspot/test/runtime/ErrorHandling on Windows,
Linux, AIx.

Kind Regards, Thomas


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