RFR: 8223504: improve performance of forall loops by better inlining of "iterator()" methods.

[Vladimir Ivanov]

> http://cr.openjdk.java.net/~skuksenko/hotspot/8223504/webrev.01/

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

+  if (callee_method->name() == ciSymbol::iterator_name()) {
+    if 
(callee_method->signature()->return_type()->is_subtype_of(C->env()->Iterator_klass())) 
{
+      return true;
+    }
+  }

The check looks too broad for me: it returns true for any method with a 
name "iterator" which returns an instance of Iterator which is much 
broader that just overrides/overloads of Iterable::iterator().

Can you elaborate, please, why did you decide to extend the check for 
non-Iterables?

Commenting on the general approach, it looks like a good candidate for a 
fist-line filter before performing a more extensive analysis. I'd prefer 
to see BCEscapeAnalyzer extended to determine that returned Iterator is 
a freshly-allocated instance and decide whether to inline or not based 
on that instead. Among java.util classes you mentioned most iterators 
are trivial, so even naive analysis should get decent results.

And then the analysis can be applied to any method which returns an 
Object to see whether EA may benefit from inlining.

What do you think?

Best regards,
Vladimir Ivanov

> On 5/7/19 11:56 AM, Aleksey Shipilev wrote:
>> On 5/7/19 8:39 PM, Sergey Kuksenko wrote:
>>> Hi All,
>>>
>>> I would like to ask for review the following change/update:
>>>
>>> https://bugs.openjdk.java.net/browse/JDK-8223504
>>>
>>> http://cr.openjdk.java.net/~skuksenko/hotspot/8223504/webrev.00/
>> The idea sounds fine.
>>
>> Nits (the usual drill):
>>
>>   *) Copyright years need to be updated, at least in bytecodeInfo.cpp
>>
>>   *) Do we need to put Iterator_klass initialization this early in 
>> WK_KLASSES_DO? It feels safer to
>> initialize it at the end, to avoid surprising bootstrap issues.
>>
>>   *) Backslash indent is off here in vmSymbols.hpp:
>>
>>   129   template(java_util_Iterator,                        
>> "java/util/Iterator")               \
>>
>>   *) Space after "if"? Also, I think you can use ciType::is_subtype_of 
>> instead here. Plus, since you
>> declared iterator in WK klasses, SystemDictionary::Iterator_klass() 
>> should be available.
>>
>>   100     if(retType->is_klass() && 
>> retType->as_klass()->is_subtype_of(C->env()->Iterator_klass())) {
>>