RFR(s): 8224193: stringStream should not use Resouce Area

[Thomas Stüfe]

Hi Goetz,

I realize my first answer was a bit short, so I'll try to extend my answer
a bit:

On Mon, May 20, 2019 at 6:06 PM Thomas Stüfe <thomas.stuefe at gmail.com>
wrote:

> Hi Goetz,
>
> thanks for the review. Remarks inline.
>
> On Mon, May 20, 2019 at 4:56 PM Lindenmaier, Goetz <
> goetz.lindenmaier at sap.com> wrote:
>
>> Hi Thomas,
>>
>> this looks good and it would be great to get rid of the
>> limitation to nested ResourceMarks.
>>
>> But I see two advantages of the resource allocation we
>> lose with your change:
>>   * Cache locality. Repeated allocations will all use the same
>>     memory in the arena keeping cache misses (even cold misses)
>>     minimal.
>
>   * The resource area is designed to have very low overhead of
>>      the memory management.
>>
>> What do you think?
>>
>>
> I think the existing design is fundamentally broken and discussing the
> theoretical performance benefits of a broken design is useless :)
>
> See bug report. Both points I made are total deal breakers. Do you see any
> way around them while still using RA?
>
>
Point 1) - sub-function-scope ResourceMarks - will "just" cause crashes and
asserts in a quasi random fashion, based on log verbosity. That is already
bad enough.

Point 2) - the resource-area-reallocation - is IMHO even more evil. There
is this notion in the VM of resizing resource area allocations (see
Arena::Arealloc()), but that only works as expected if the array in
question is still at the top of the resource area stack. If a subsequent
allocation happened, that "reallocation" is instead a new allocation of a
greater size, abandoning the old array which is still on the RA stack, and
memcpy()ing all the content over.

Consider this:

stringStream ss;
for (.....) {
  some_object->print_on(&ss);
}

SomeObject::print_on(outputStream* os) {
   char* x = bla.as_string();
   os->print("%s", x);
}

stringStream gets allocated with a backing buffer of 256 chars.

bla.as_string() allocates a RA temp buffer, which now is at the top of the
stack. If the subsequent os->print() causes an internal buffer overflow, it
will attempt to realloc the backing buffer - first to 1024 chars. We have
no local ResourceMark, so no crash here, but the buffer will be physically
duplicated and we still retain the old copy. RA stack looks like this:

---
first stringStream backing buffer (256 char)
---
temp buffer from Bla::as_string()
---
second stringStream backing buffer (1024 char), content memcpy() ed over
from the first buffer
---

Pattern will repeat every time this constellation happens. Memory will grow
way more quickly as if we would just realloc via C heap.

At this point cache locality is not really a concern anymore, yes? :)

In my eyes any allocation which needs to be enlarged is not a candidate for
RA allocation. If it needs to be enlarged inside sub functions this makes
it even worse.

Cheers, Thomas


> ... I like the optimization of as_String() you propose in the other
>> change :)
>>
>
> Thank you.
>
>
>>
>> Best regards,
>>   Goetz.
>>
>
> Thanks, Thomas
>
>
>>
>> > -----Original Message-----
>> > From: hotspot-runtime-dev <hotspot-runtime-dev-bounces at openjdk.java.net
>> >
>> > On Behalf Of Thomas Stüfe
>> > Sent: Montag, 20. Mai 2019 15:25
>> > To: Hotspot dev runtime <hotspot-runtime-dev at openjdk.java.net>
>> > Subject: RFR(s): 8224193: stringStream should not use Resouce Area
>> >
>> > Hi all,
>> >
>> > may I please have reviews for the following bug fix:
>> >
>> > issue: https://bugs.openjdk.java.net/browse/JDK-8224193
>> > cr:
>> >
>> http://cr.openjdk.java.net/~stuefe/webrevs/8224193-stringstream-shall-not-
>> > use-resource-array/webrev.00/webrev/
>> >
>> > In short, stringStream uses resource area which is a poor choice -
>> > depending on the logging we do, this may crash or assert and if it does
>> not
>> > it at least wastes memory.
>> >
>> > Note that I kept the change as simple as possible. There are other
>> possible
>> > improvements beside pure code cleanup, for which I opened follow up
>> issues
>> > https://bugs.openjdk.java.net/browse/JDK-8224212 and
>> > https://bugs.openjdk.java.net/browse/JDK-8224213.
>> >
>> > Thanks, Thomas
>>
>