RFR: JDK-8259710: Inlining trace leaks memory [v2]

[Tobias Hartmann]

On Tue, 19 Jan 2021 06:06:17 GMT, Thomas Stuefe <stuefe at 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