RFR(L): 8005071: Incremental inlining for JSR 292

[Christian Thalinger]

On Dec 19, 2012, at 10:01 AM, Vladimir Kozlov <vladimir.kozlov at oracle.com> wrote:

> On 12/19/12 9:31 AM, Roland Westrelin wrote:
>>> While we are waiting Christian review, could you do some clean up in your changes?
>> 
>> Sure.

src/share/vm/opto/bytecodeInfo.cpp:

-  if (callee_method->force_inline() || callee_method->should_inline()) {
+  if ( callee_method->should_inline()) {

Are ForceInline methods always inlined after this change?  Because there are more checks after that line.

I'm not so happy with your additional boolean arguments:

delay
no_late
not_const

We probably can't live without them (can we?) but more meaningful names would be good.

src/share/vm/opto/compile.cpp:

+  // clean up the late inline lists
+  for (int j = 0; j < 2; j++) {
+    int shift = 0;
+    GrowableArray<CallGenerator*>* inlines = j == 0 ? &_string_late_inlines : &_late_inlines;

Why 2?  Maybe you should move that loop into a method and call it with the arrays as an argument.

src/share/vm/opto/compile.hpp:

+  uint                          _nb_mh_late;          // number of method handle late inlining still pending

Usually we have a _nof prefix for "number of" but can't we just call it like that?

+  uint                          _number_of_mh_late_inlines;          // number of method handle late inlining still pending

I don't like these abbreviations.

+  void Incremental_Inline(PhaseIterGVN& igvn, bool eager = false);
+  void String_Inline(bool parse_time);

Let's not add new camel-case methods.

-- Chris

>> 
>>> Remove IncrementalInlineAfterOpts flag since it is unused. Also may be IncrementalInlineOneByOne and AlwaysIncrementalInline flags because it looks like they were used just for experiment.
>> 
>> AlwaysIncrementalInline is useful for stress testing so I'd like to keep it.
> 
> OK.
> 
>> 
>>> But I don't see the flag which control new code usage. We should have one to be able switch new code totally OFF. MaxNodeLimit value should depend on that flag.
>> 
>> -XX:LiveNodeCountInliningCutoff=0 turns the new code off but it doesn't adjust MaxNodeLimit. Do you think another flag is needed or checking for LiveNodeCountInliningCutoff=0 at startup and setting MaxNodeLimit to the former value is sufficient?
> 
> It is not obvious way. I would prefer separate product flag -XX:+IncrementalInline. Also verify that incremental inlining is off if we switch of all inlining -XX:-Inline.
> 
> Thanks,
> Vladimir
> 
>> 
>>> Move whole later inlining loop into Incremental_Inline(igvn) to simplify code in Optimize(). Leave original igvn.optimize() call where it was before. In Incremental_Inline(igvn) do sequence:
>>> 
>>> while(..) {
>>> if (needed) PhaseIdealLoop
>>> late inlining
>>> igvn
>>> }
>> 
>> Ok.
>> 
>>> Add comment, explaining why you need low_live_nodes.
>> 
>> Ok.
>> 
>>> bytecodeInfo.cpp:
>>> 
>>> 213   if ( callee_method->should_inline()) {
>>>          ^ remove space
>> 
>> Ok.
>> 
>>> Move inlining_progress() up after clear_inlining_progress():
>>> 
>>> +  void        clear_inlining_progress()         { _inlining_progress = false; }
>>> +  int               inlining_incrementally() const { return _inlining_incrementally; }
>>> +  void          set_inlining_incrementally()       { _inlining_incrementally = true; }
>>> +  int               inlining_progress() const   { return _inlining_progress; }
>> 
>> Ok.
>> 
>>> I still don't like new dead code elimination code but with time constrain I may gave in. We should not crush without that code - and it should be fixed and not covered by this new code.
>> 
>> Ok. I welcome suggestions and I'll think about it more.
>> 
>> Roland.
>>