RFR(s): 8220394: bufferedStream does not honor size limit
thomas.stuefe at gmail.com
Sun Mar 17 08:19:21 UTC 2019
Hi David, sorry for the late response,
On Mon, Mar 11, 2019 at 8:56 AM David Holmes <david.holmes at oracle.com>
> 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:
> > 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
> > 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
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
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.
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
More information about the hotspot-runtime-dev