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

[Thomas Stüfe]

Hi David,


On Wed, Dec 16, 2015 at 7:46 AM, David Holmes <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.


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

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