RFR(xxs): 8145410: OutputStream::fill_to() does not work for error log

[David Holmes]

On 16/12/2015 5:26 PM, Thomas Stüfe wrote:
> Hi David,
>
>
> On Wed, Dec 16, 2015 at 7:46 AM, David Holmes <david.holmes at oracle.com
> <mailto:david.holmes at oracle.com>> wrote:
>
>     Hi Thomas,
>
>     I'll need a second opinion on this one :)
>
>     On 16/12/2015 12:59 AM, Thomas Stüfe wrote:
>
>         Hi all,
>
>         please review and sponsor this tiny change.
>
>         outputStream::fill_to() does not work as expected for the
>         staticBufferStream because its position never gets updated. That
>         means that
>         fill_to() does not work in error reporting.
>
>
>     Okay I think I see what you are doing with the call to
>     update_position, but shouldn't all staticBufferStream methods be
>     delegating to the _outer_stream? I confess I'm unclear the roles of
>     the buffer and the outer_stream in this abstraction.
>
>
> I do not really like this delegation coding, but it is as it is for now.
> If we wanted to relegate all functionality to the inner stream, we would
> have to overwrite more functions in the staticBufferStream - any
> functions really which use one of the _position, _newlines, _precount
> members - to not use the ones from the outer class but the one from the
> inner class.
>
> But I also think that delegation is a bad fit here. The only reason
> staticBufferStream exists is because it uses a user-provided buffer as
> scratch-buffer for do_vsnprintf() to prevent large stack allocations.
> The comment is also misleading, it was difficult at first to tell apart
> the difference between staticBufferStream, stringStream and bufferedStream.
>
> For bufferedStream and stringStream "buffer" means the real output
> buffer, not the scratch buffer for vsnprintf. Otherwise I am hazy on the
> difference between those two - the only difference I could spot is that
> bufferedStream seems to use C-Heap, stringStream uses resource area
> buffer. Both cases support growing on demand if needed.
>
> I think the scratch buffer management would be way better incorporated
> into outputStream itself. For instance, provide methods to provide the
> class with a user provided scratch buffer - and if none is given, use
> stack space. That would get rid completely of staticBufferStream.
>
> There is a lot of redundant coding in ostream.cpp, and this would
> benefit from cleaning up.
>
> But I think my change, for now, is the least invasive fix without larger
> redesign.

Okay. The only existing use of staticBufferStream is in the vmError 
routines. Does this change impact though usages at all, or does it only 
affect uses of fill_to ?

>
>     Aside: Looking at these other uses of update_position:
>
>     void fileStream::write(const char* s, size_t len) {
>        if (_file != NULL)  {
>          // Make an unused local variable to avoid warning from gcc 4.x
>     compiler.
>          size_t count = fwrite(s, 1, len, _file);
>        }
>        update_position(s, len);
>     }
>
>     void fdStream::write(const char* s, size_t len) {
>        if (_fd != -1) {
>          // Make an unused local variable to avoid warning from gcc 4.x
>     compiler.
>          size_t count = ::write(_fd, s, (int)len);
>        }
>        update_position(s, len);
>     }
>
>     shouldn't they be inside the if statement ??
>
>
> Not sure but I think in both cases it does not matter, apart from
> uselessly burning cycles. Because there is no way to reopen a file if it
> fails to open in the constructors of fdStream or fileStream. And if it
> cannot be ever reopened, nothing will ever be written for this stream
> again, so bookkeeping newlines and position has no purpose.

Looks weird to me :)

Thanks,
David

> Kind Regards, Thomas
>
>     Cheers,
>     David
>     -----
>
>
>
>         (I need this function because I am implementing an AIX-specific
>         callstack
>         dumper and with the broken fill_to the output looks quite ugly)
>
>         Bug: https://bugs.openjdk.java.net/browse/JDK-8145410
>         webrev:
>         http://cr.openjdk.java.net/~stuefe/webrevs/8145410-fill_to-errorlog/webrev.00/webrev/
>
>         Thanks!
>
>         Thomas
>
>