[9] RFR(M): 8129847: Compiling methods generated by Nashorn triggers high memory usage in C2

Thu Nov 26 13:41:11 UTC 2015

Hi Vladimir,

On 11/26/2015 02:19 PM, Vladimir Ivanov wrote:
> Overall, looks good.

thank you for the feedback!

>
> I have 2 questions:
> (1) What benefits do you see from _parse_idx? Is it still useful if we 
> do multiple renumberings?

_parse_idx is certainly useful with a single renumbering as it allows 
you to determine what the _idx of a Node was up to the point where 
cleanup_expensive_nodes() is finished.

If we were to do two renumberings (let's say R1 and R2), matching a 
node's _idx as it is after R2 is finished to the same node's _idx as it 
was between R1 and R2 might be difficult. But I would not recommend 
performing multiple renumberings (at least not for now), please see 
reasons below.

>
> (2) I see some benefits in performing renumbering multiple times 
> during compilation. For example, incremental inlining produces lots of 
> dead nodes (e.g. JDK-8059241 [1]), so it should further reduce peak 
> memory usage in invokedynamic/MethodHandle heavy scenarios.
>
> Dead node elimination, IGVN, and loop optimization passes are already 
> performed (see Compile::inline_incrementally). If there's large enough 
> gap between unique & live node counts, renumbering can be performed as 
> well.

Yes, that is a valid point. I have been experimenting with a version 
that performs renumbering at 6 different places instead of only one:

Compile::Compile()
Compile::inline_boxing_calls()
Compile::inline_incrementally_one()
Compile::inline_incrementally()
Compile::inline_string_calls()
Compile::Optimize()

(The current version performs renumbering only in Compile::Optimize().)

With that version we get 46% reduction of the reproducer's resident set 
size (RSS) instead of 39% (the current version). The reduction with 
Nashorn/Octane differs as well: 8% vs. 5% (current version).

But the overhead due to compilation is higher as well and that results 
in a performance regression of 3% with SPECjvm2008-MPEG. That is why I 
decided to go with the current version that does not regress.

So I would like to first get this version in and see if we get problems 
(bugs or regressions). If all looks fine, we can think of enabling 
PhaseRenumberLive at other places as well (not necessarily all 6 from 
above, only those that bring good benefits for a reasonable cost).

Thank you and best regards,

Zoltan

>
> Best regards,
> Vladimir Ivanov
>
> [1] https://bugs.openjdk.java.net/browse/JDK-8059241
>
> On 11/26/15 3:39 PM, Zoltán Majó wrote:
>> Hi Tobias,
>>
>>
>> On 11/26/2015 09:41 AM, Tobias Hartmann wrote:
>>> Hi Zoltan,
>>>
>>> nice improvements! The implementation looks good to me.
>>
>> thank you for your feedback!
>>
>>> Here are some questions / suggestions:
>>> - In the constructor of PhaseRenumberLive you check the value of
>>> RenumberLiveNodes and pass the corresponding PhaseNumber to
>>> PhaseRemoveUseless. But shouldn't RenumberLiveNodes always be true
>>> here? Maybe you should add an assert.
>>
>> You are right, RenumberLiveNodes must be always true when performing a
>> PhaseRenumberLive. I removed the check and added an assert. (I had that
>> check from an earlier version that, depending on the value of the
>> RenumberLiveNodes, could perform a use PhaseRenumberLive in place of a
>> PhaseRemoveUseless.)
>>
>>> - The comment says that "PhaseGVN::_nodes" is not updated because it's
>>> always empty when this phase is used. Should we add an assert here as
>>> well?
>>
>> An assert does not hurt here so I added one that checks if the size of
>> PhaseGVN::_nodes is 0 when PhaseRenumberLive starts. As a I side effect,
>> I had to change the type of PhaseGVN::_nodes from Node_Array to
>> Node_List (a subtype of Node_Array that keeps a count of the number of
>> elements).
>>
>> Here is the updated webrev:
>> [9]: http://cr.openjdk.java.net/~zmajo/8129847-9/webrev.05/
>> [8u]: http://cr.openjdk.java.net/~zmajo/8129847-8u/webrev.05/
>>
>> Testing:
>> - JPRT;
>> - locally (linux_x64) executed all hotspot test, all tests pass that
>> pass with the unmodified build.
>>
>> Thank you and best regards,
>>
>>
>> Zoltan
>>
>>>
>>> Best,
>>> Tobias
>>>
>>>
>>> On 25.11.2015 15:57, Zoltán Majó wrote:
>>>> Hi,
>>>>
>>>>
>>>> please review the patch for 8129847.
>>>>
>>>> https://bugs.openjdk.java.net/browse/JDK-8129847
>>>>
>>>> Problem: JDK-8014959 and JDK-8058148 have increased the value of the
>>>> LiveNodeCountInliningCutoff and MaxNodeLimit thresholds. The
>>>> thresholds closely affect the number of unique compiler nodes within
>>>> compilations. As the size of many of the C2 compiler's data
>>>> structures is proportional to the number of unique nodes, the changed
>>>> thresholds increase the memory usage of the C2 compiler. In some
>>>> cases (e.g., when compiling Nashorn-generated methods), the memory
>>>> usage increases significantly.
>>>>
>>>> Solution: This changeset proposes to add a new compiler phase,
>>>> PhaseRenumberLive, that (1) identifies live nodes and then (2)
>>>> assigns a new ID to each node. Each node is assigned a distinct Node
>>>> IDs from the interval [0, x), where 'x' is the number of live nodes.
>>>> PhaseRenumberLive updates the compiler's unique count to 'x'.
>>>> Decreasing the compiler's count of unique nodes reduces the
>>>> compiler's memory usage.
>>>>
>>>> Webrev: I created a webrev for both 8u and 9 because the reproducer
>>>> attached with the bug report runs only with 8u:
>>>> [9]: http://cr.openjdk.java.net/~zmajo/8129847-9/webrev.04/
>>>> [8u]: http://cr.openjdk.java.net/~zmajo/8129847-8u/webrev.04/
>>>>
>>>> Testing:
>>>> - JPRT
>>>> - Octane/Nashorn
>>>>
>>>> Evaluation:
>>>> - reduction of the number of unique nodes: see following table with
>>>> top 5 methods in reproducer (methods were ranked in decreasing order
>>>> of their maximum unique count):
>>>> http://cr.openjdk.java.net/~zmajo/8129847-8u/node_count.html
>>>> - reduction of resident set size (RSS) of the VM: 39% with
>>>> reproducer, 5% on average with Octane/Nashorn;
>>>> - no performance regression (Octane/Nashorn, SPECjvm2008).
>>>>
>>>> Thank you and best regards,
>>>>
>>>>
>>>> Zoltan
>>>>
>>