> 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()
-------------
Changes:
- all: https://git.openjdk.java.net/jdk/pull/2063/files
- new: https://git.openjdk.java.net/jdk/pull/2063/files/4045f2f8..b52b140b
Webrevs:
- full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2063&range=01
- incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2063&range=00-01
Stats: 2375 lines in 4 files changed: 2365 ins; 0 del; 10 mod
Patch: https://git.openjdk.java.net/jdk/pull/2063.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/2063/head:pull/2063
PR: https://git.openjdk.java.net/jdk/pull/2063
