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

[Zoltán Majó]

Hi Aleksey,


On 11/26/2015 02:54 PM, Aleksey Shipilev wrote:
> Hi Zoltan,
>
> This is a very interesting approach indeed!

thank you for the feedback and suggestions!

>
> On 11/25/2015 05:57 PM, Zoltán Majó wrote:
>> [9]: http://cr.openjdk.java.net/~zmajo/8129847-9/webrev.04/
> I have a few specific comments:
>
> a) The interaction between Remove_Useless and Renumber_Live is unclear
> from the code like this:
>
>    47     Remove_Useless,             // Remove useless nodes
>    48     Renumber_Live,              // First, remove useless nodes from
> the graph. Then, renumber live nodes.
>
> It seems that Renumber_Live completely subsumes the Remove_Useless?
> Maybe a better name would be Remove_Useless_And_Renumber to capture
> that? Or the inclination is that Remove_Useless is a cleanup action that
> runs multiple times anyway? If so, there seem to be no reason to mention
> it in the phase description.

Remove_Useless can (and is) performed independently of Renumber_Live. 
But Renumber_Live performs a Remove_Useless before renumbering. To make 
that clearer, I updated the constant's name to 
Remove_Useless_And_Renumber, as you suggested.

>
> b) Can you add the compilation time counters: TracePhase, &timers, etc.,
> so that a new phase time is visible in -XX:+CITime?

Good idea, I added the compilation time counters.

>
> c) Does this renumbering break Ideal Graph Visualizer, i.e. does IGV
> depend on the node indexes stability in any way?

I tried and renumbering does not break IGV.

>
> d) It seems odd to have a check for RenumberLiveNodes in
> PhaseRenumberLive::PhaseRenumberLive super constructor call, when all
> calls to PhaseRenumberLive seem to be predicated externally with it?
>
>   472 PhaseRenumberLive::PhaseRenumberLive(...
>   473                                      ...
>   474                                      ...) :
>   475   PhaseRemoveUseless(gvn, worklist, RenumberLiveNodes ?
> Renumber_Live : Remove_Useless) {

Yes, that is a valid point, Tobias also noted that. The updated webrev 
corrects that issue.

Here is the updated webrev:
[9]: http://cr.openjdk.java.net/~zmajo/8129847-9/webrev.06/
[8u]: http://cr.openjdk.java.net/~zmajo/8129847-8u/webrev.06/

Testing: JPRT.

Thank you and best regards,


Zoltan


>
>
> Thanks,
> -Aleksey
>