On Tue, 19 Jan 2021 06:06:17 GMT, Thomas Stuefe <stu...@openjdk.org> wrote:
>> We see C-heap leaking, originating in C2:
>>
>> 1434777 [0x00007f58214af461] stringStream::stringStream(unsigned long)+0x55
>> 1434778 [0x00007f5820c6db3e]
>> Compile::PrintInliningBuffer::PrintInliningBuffer()+0x6c
>> 1434779 [0x00007f5820c724f1]
>> GrowableArrayWithAllocator<Compile::PrintInliningBuffer,
>> GrowableArray<Compile::PrintInliningBuffer> >::grow(int)+0x163
>> 1434780 [0x00007f5820c7160e]
>> GrowableArrayWithAllocator<Compile::PrintInliningBuffer,
>> GrowableArray<Compile::PrintInliningBuffer> >::insert_before(int,
>> Compile::PrintInliningBuffer const&)+0x82
>> 1434781 [0x00007f5820c6aaf2] Compile::print_inlining_push()+0x70
>>
>> accumulating over the course of days to some hundred MB at a customer site
>> where inline tracing was active.
>>
>>
>> Analysis:
>>
>>
>> To build up a comprehensive inlining trace, c2 keeps trace snippets in an
>> internal list and assembles them at print time. These are stringStream's,
>> contained in PrintInliningBuffer:
>>
>> GrowableArray<PrintInliningBuffer>* _print_inlining_list;
>> ...
>> class PrintInliningBuffer : public ResourceObj {
>> private:
>> CallGenerator* _cg;
>> stringStream* _ss;
>>
>> public:
>> PrintInliningBuffer()
>> : _cg(NULL) {
>> _ss = new stringStream();
>> }
>>
>> void freeStream() {
>> _ss->~stringStream(); _ss = NULL; }
>> ...
>> };
>>
>> With [JDK-8224193](https://bugs.openjdk.java.net/browse/JDK-8224193),
>> stringStream was changed to use C-heap instead of ResourceArea for its
>> internal buffer. This means one cannot just omit stringStream destruction
>> anymore - even where stringStream objects themselves live in RA, their
>> internal buffers don't, they live in C-Heap. To clean up properly,
>> ~stringStream() must be called.
>>
>> Those `PrintInliningBuffer` objects are kept _by value_ in a GrowableArray
>> `Compile::_print_inlining_list`. Normally, if an object is kept by value, it
>> needs to implement correct copy- and destruction-semantics.
>> PrintInliningBuffer does not do that and instead relies on manual cleanup
>> (`PrintInliningBuffer::freeStream()`) - I assume the authors did not want to
>> deal with reference counting the contained stringStream on copying.
>>
>> That however is a problem because GrowableArray creates internally temporary
>> objects of the item type to pre-populate its array - its whole capacity -
>> when it grows:
>>
>>
>> template <typename E, typename Derived>
>> void GrowableArrayWithAllocator<E, Derived>::grow(int j) {
>>
>> ...
>> for ( ; i < this->_len; i++) ::new ((void*)&newData[i])
>> E(this->_data[i]);
>> for ( ; i < this->_max; i++) ::new ((void*)&newData[i]) E(); <<<<<<<
>> for (i = 0; i < old_max; i++) this->_data[i].~E();
>> ...
>> }
>>
>> this it does for the whole new capacity, which means more objects get
>> created than have been added; if the caller does not fill the GrowableArray
>> up to its capacity, it never knows about the excess objects created. This is
>> normally not a problem since all these objects are destructed by
>> GrowableArray in `void GrowableArrayWithAllocator<E,
>> Derived>::clear_and_deallocate()`. But since PrintInliningBuffer does not
>> implement a destructor, this has no effect.
>>
>> PrintInliningBuffer instead relies on manual cleanup of the stringStreams:
>> at the end of the compile phase, it calls
>> `PrintInliningBuffer::freeStream()` on all buffers it thinks are contained
>> in the array:
>>
>> assert(_print_inlining_list != NULL, "process_print_inlining should be
>> called only once.");
>> for (int i = 0; i < _print_inlining_list->length(); i++) {
>> ss.print("%s", _print_inlining_list->adr_at(i)->ss()->as_string());
>> _print_inlining_list->at(i).freeStream();
>> }
>>
>> but this of course leaves the excess objects untouched (objects whose index
>> is array length >= index >= array capacity).
>>
>> -----------
>>
>> There are several ways to fix this:
>>
>> 1) implement correct destructor- and copy semantics for PrintInliningBuffer.
>> This would work but is not optimal since we would still pay for creation of
>> unnecessary PrintInliningBuffer objects when the array is prepopulated on
>> grow()
>>
>> 2) delay construction of `PrintInliningBuffer::_ss` until the stream is
>> actually used for writing into. This would would mean we have to check the
>> stringStream for NULL before using it.
>>
>> 3) Just let the PrintInliningBuffer live in C-heap instead of the RA. Its
>> only a small shell anywhere now that the stringStream buffer lives in
>> C-heap. Instead, let PrintInliningBuffer contain the stringStream as a
>> member, allocate PrintInliningBuffer from C-heap, and change the list to
>> contain pointers to PrintInliningBuffer, not the object itself. This removes
>> all that unnecessary copying, removes creation of temporary objects, and
>> simplifies the code. Having to free those objects is no big deal since we
>> free the stringStream objects manually anyway.
>>
>> I went with (3). I also decreased the default stringStream buffer size for
>> this scenario since when testing I found out that many trace snippets are
>> below 128 bytes.
>>
>> Note that (3) is not only simpler, but also more efficient: many of the
>> PrintInliningBuffer objects are inserted into the middle of the
>> _print_inlining_list; GrowableArray does shift all higher items up to
>> provide space for the new item. If those items are simple pointers instead
>> of whole objects, less memory is moved around.
>>
>> Tests:
>> - github actions
>> - Nightlies at SAP
>> - I manually tested the patch and compared the NMT output of a tracing
>> scenario to verify that the leak went away and no other numbers changed
>>
>> Thanks, Thomas
>
> Thomas Stuefe has updated the pull request incrementally with one additional
> commit since the last revision:
>
> change return parameter of Compile::print_inlining_current()
Thanks for updating. Looks good but you accidentally pushed
`make/hs_err_pid10680.log`, `make/replay_pid10680.log` :)
Best regards,
Tobias
-------------
Changes requested by thartmann (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/2063
