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

[Jan Lahoda]

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.

>
>   *
>
>     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
>
> ​