RFR(xs): 8146905 - cleanup ostream, staticBufferStream

[Thomas Stüfe]

Thank you, David!



On Thu, Jan 28, 2016 at 5:26 AM, David Holmes <david.holmes at oracle.com>
wrote:

> Thanks Thomas - push under way.
>
> Please note the changeset comment must have form:
>
> <bug number>: <description>
>
> Cheers,
> David
>
> On 27/01/2016 9:05 PM, Thomas Stüfe wrote:
>
>> 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
>> <mailto: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 <mailto: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 <mailto: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 <mailto: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
>>
>>
>>
>>