RFR: JDK-8193717: Import resolution performance regression in JDK 9

[Maurizio Cimadamore]

Few comments:

  *

    the use of an array as a value in the map seems to cause too much GC
    overhead - e.g. the array has to be re-allocated from scratch each
    time a new subscope is added for a given name; wouldn’t a List be
    better? I assume that element count will be low, so the linked list
    overhead might be negligible (and better than allocating a
    bigger-than-needed array, as for an ArrayList).

  *

    this code looks dubious:

|Scope[] scopes = name2Scopes.getOrDefault(name, new Scope[0]); + if 
(scopes == null) + return Collections.emptyList(); |

When can ‘scopes’ ever be null? If there’s no entry for given name, the 
use of getOrDefault guarantees that a zero-element array is returned; so 
the only option would be if ‘null’ is explicitly stored as a map value; 
but looking at the code, the stores are done here:

|Scope[] existing = name2Scopes.get(name); + if (existing != null) + 
existing = Arrays.copyOf(existing, existing.length + 1); + else + 
existing = new Scope[1]; + existing[existing.length - 1] = newScope; + 
name2Scopes.put(name, existing); |

So, again, this should guarantee that the value associated with a ‘put’ 
is always non-null?

Maurizio

On 21/05/18 12:24, Jan Lahoda wrote:

> Hi,
>
> A webrev updated to the current situation, and with a comment 
> explaining the need for the HashMap is here (as suggested offline):
> http://cr.openjdk.java.net/~jlahoda/8193717/webrev.01/
>
> Any feedback is welcome.
>
> Thanks,
>     Jan
>
> On 2.3.2018 17:52, Jan Lahoda wrote:
>> Hi,
>>
>> Having a source with a lot of imports can lead to a long compilation
>> time, as the imports are lazily searched on each query to the Scope. But
>> at least for named imports, the name is known, and can be used for quick
>> filtering. (Sadly, for on-demand imports/package imports, this is much
>> more difficult, and this patch does not improve them.)
>>
>> The principle of the patch is to have a map from name to scopes that may
>> contain the given name in Scope.NamedImportScope.
>>
>> Also, the "current-file top-level scope"
>> (JCCompilationUnit.toplevelScope) is no longer appended to the named
>> import scope, but rather the relevant lookup is enhanced to query it as
>> well if needed. This seems cleaner that trying to append the scope to
>> the fast name lookup.
>>
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8193717
>> Webrev: http://cr.openjdk.java.net/~jlahoda/8193717/webrev.00/
>>
>> Liam, could you please check if this helps with your real-world case?
>>
>> How does this look?
>>
>> Thanks,
>>       Jan

​
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/compiler-dev/attachments/20180521/32417a54/attachment.html>