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

[Thomas Stüfe]

On Sun, Mar 17, 2019 at 9:19 AM Thomas Stüfe <thomas.stuefe at gmail.com>
wrote:

> 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.
>
>
And I see now that stringStream uses ResourceArea, which is not good and
should be fixed too. We did so already for LogStreams:
https://bugs.openjdk.java.net/browse/JDK-8181917.

Sigh.

..Thomas


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