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

[Maurizio Cimadamore]

On 21/05/18 14:15, Jan Lahoda wrote:
> Hi Maurizio,
>
> Thanks for the comments!
>
> On 21.5.2018 13:44, Maurizio Cimadamore wrote:
>> 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).
>
> The idea here was that (presumably) most of these arrays will have 
> just one element (so no resizing needed), and it is easier to keep the 
> order in which the Scopes were added using arrays than with 
> com.sun.tools.javac.util.List. But can be changed to a linked list if 
> needed.
I trust your call here - I just wanted to point out that this code might 
regress badly if the assumption it relies upon is violated.

Maurizio
>
>>
>>   *
>>
>>     this code looks dubious:
>>
>> |Scope[] scopes = name2Scopes.getOrDefault(name, new Scope[0]); + if
>> (scopes == null) + return Collections.emptyList(); |
>
> Oops, right, that is unnecessary. I'll change the getOrDefault to get 
> (so that if there's no mapping, it method will end up quickly and not 
> create the compound iterator), if that's OK.
>
> Jan
>
>>
>> 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
>>
>> ​