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

[Vladimir Ivanov]

Overall, looks good.

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

(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.

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
>>>
>