RFR(s): 8220394: bufferedStream does not honor size limit

[Thomas Stüfe]

Hi David, sorry for the late response,

On Mon, Mar 11, 2019 at 8:56 AM David Holmes <david.holmes at oracle.com>
wrote:

> Hi Thomas,
>
> On 11/03/2019 4:19 am, Thomas Stüfe wrote:
> > Hi all,
> >
> > may I have please reviews for this fix:
> >
> > bug: https://bugs.openjdk.java.net/browse/JDK-8220394
> > cr:
> >
> http://cr.openjdk.java.net/~stuefe/webrevs/8220394--bufferedstream-does-not-honor-size-limit/webrev.00/webrev/index.html
> >
> > When playing around with a new jcmd addition, I found that
> bufferedStream -
> > which is used quite a bit in the VM - in its dynamic, self-allocating
> form
> > does not honor the buffer size limit. So faulty or just plain verbose
> > printing code can cause high memory usage.
> >
> > First I wanted to keep the change minimal but I found a number of things
> > wrong with bufferedStream, mainly the fact that "bufmax" was actually
> > used/misused as a flush threshold, for the use of the child class
> > "networkStream". So I decided to separate the two - buffer size max and
> > flush threshold - cleanly.
>
> Hmmm my take on this was that bufferedStream was erroneously not
> implementing a flush that actually flushes and resets the buffer. It
> seems somewhat bizarre to have bufferedStream provide bufmax only for
> its subclass. Not sure we need separate flush and size thresholds -
> especially given that flush has been a no-op (we obviously don't care
> about when to flush).
>

Yes this seems weird. I think the original intend was to have a whole lot
of sub classes and a way to trigger flushes on all of them. But as it is
now, networkStream is the sole child class.

I have the feeling that bufferedStream is misused in a couple of places
where one just wanted actually to write to a temp buffer for later output
(e.g. codeHeapState.cpp, JFR, ..). And then, when it is time to print it,
they use as_string() on the stream which imposes another copy operation to
ResourceArea.

All these places would be better served with stringStream.

In addition I see "bufferedStream*" arguments in a lot of places where the
base "outputStream*" would be better - no actual reason to impose use of a
bufferedStream.

I believe the clean, thorough solution would be to bin bufferedStream -
merge its functionality with networkStream - and replace all those cases
where it is misused with stringStream.

But this is from the top of my head. There may be reasons I do not see.


> There would still be a bug with regard to the resizing strategy and
> "bufmax" due to re-doubling for small values, but that seems easily fixed.
>
> > I also renamed the members to be conform with modern hotpsot code and
> > changed ctors to use initializer lists.
> >
> > Note that when examining the streams, I found that they do not seem to
> > explicitly zero-terminate. I was surprised by that, is that intentional?
>
> I'm confused - why would a (non-string)stream zero-terminate?
>

Yes you are right. As I wrote, I see it getting misused as stringStream,
which originally confused me.


>
> Cheers,
> David
>
>

As for this patch  - I withdraw it for now. I may do a very simple stop-gap
patch since having an outputstream grow without limit seems bad enough to
fix quickly, without waiting for the big refactoring.

Cheers, Thomas



> >
> > Cheers, Thomas
> >
>