RFR(xs): 8146905 - cleanup ostream, staticBufferStream

[David Holmes]

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