RFR 8228675: Resolve.findMethod doesn't cache interface type calculation

[Maurizio Cimadamore]

I've been catching up on this discussion - having written this code, I 
know this is tricky and also hard to optimize. Some efforts were already 
put in trying to minimize the lookups, but it seems now you are being 
affected by the cost of calling types.closure, which I understand.

Looking at the code again, I wonder if, before doing something, I'd like 
to step back and look at what the code is doing - specifically, I'm 
looking at this:

for (Type itype : itypes[iphase2.ordinal()]) {
                 if (!itype.isInterface()) continue; //skip j.l.Object 
(included by Types.closure())
                 if (iphase2 == InterfaceLookupPhase.DEFAULT_OK &&
                         (itype.tsym.flags() & DEFAULT) == 0) continue;


So there are two cases where the type in the itypes array is actually 
not even looked at:

* the type is Object - ok, this is a corner case - because closure() 
will return also Object, fine

* if we're looking for defaults, and the type we have on our hands does 
not have the DEFAULT flag set, quit the search

Now it would be tempting to say that if a type doesn't have the DEFAULT 
flag set, then we can avoid adding its closure to the set - but that's 
wishful thinking; the flag is (currently) only set for interfaces that 
declare some default methods themselves. So, even if an interface 
doesn't declare any default method, we still have to search its 
superinterfaces, as some of those might.

And this is where the big lookup cost associated with default methods is 
coming from - we have to scan all superinterfaces in case _one_ might 
have some default in it that is more specific than the method we have 
already found.

I think one possible way to optimize this algorithm would be to actually 
do the opposite - e.g. to mark (as we scan them) interfaces whose 
hierarchies are NOT known to have any default methods in them. If we had 
such a detection mechanism, we could then prune entire families of 
interfaces (and their parents) from the findMethod lookup - which would 
bring back lookup performances with what we had before 8 (I'm assuming 
that was fast enough for the use cases brought up here?).

If you think that such an optimization is likely to give good results, 
then I'd rather invest on that: since here we'd simply require a flag to 
cache the interface hierarchy status w.r.t. default methods, I'm less 
worried that the caching mechanism will backfire and turn into cases of 
horrible memory footprint regression (we have seen that happening in the 
past with cases like these).

Thoughts?

Cheers
Maurizio

On 07/08/2019 11:25, Ron Shapiro wrote:
> Made the change for a WeakHashMap: 
> http://cr.openjdk.java.net/~ronsh/8228675/webrev.01/.
>
> Perhaps it would make better sense for this to be a field on 
> ClassType? That would avoid the need for managing a cache with weak keys.
>
> Do you have any guidance on how you approach tradeoffs in memory vs. 
> speed?
>
> On Mon, Aug 5, 2019 at 7:25 PM Vicente Romero 
> <vicente.romero at oracle.com <mailto:vicente.romero at oracle.com>> wrote:
>
>     not sure if we should add yet another cache to javac but in any
>     case it should be implemented with a: WeakHashMap,
>
>     Thanks,
>     Vicente
>
>     On 8/5/19 10:32 AM, Ron Shapiro wrote:
>>     Fair question.
>>
>>     github.com/google/dagger <http://github.com/google/dagger>
>>     generates implementations of interfaces that implement a
>>     dependency injection graph. Sometimes, though not always, these
>>     interfaces have dozens if not hundreds of (super)interfaces which
>>     allow for builds to be sharded: each subsection of the build
>>     refers to the superinterface and then at the root of the build
>>     there is a union of them all. Naturally, this means this must be
>>     rebuilt on most/all changes to any subproject of the build.
>>
>>     בתאריך יום ב׳, 5 באוג׳ 2019, 17:05, מאת Vicente Romero
>>     ‏<vicente.romero at oracle.com <mailto:vicente.romero at oracle.com>>:
>>
>>         Hi Ron,
>>
>>         Just out of curiosity, what is the context in which this
>>         issue arises? Naturally consuming more memory we can speed up
>>         javac but this can lead to other problems too.
>>
>>         Thanks,
>>         Vicente
>>
>>         On 8/5/19 8:24 AM, Ron Shapiro wrote:
>>>         Friendly ping
>>>
>>>         בתאריך שבת, 27 ביולי 2019, 0:05, מאת Ron Shapiro
>>>         ‏<ronshapiro at google.com <mailto:ronshapiro at google.com>>:
>>>
>>>             Hi,
>>>
>>>             Please review this change to speed up Resolve.findMethod
>>>             for large classes that have large numbers of
>>>             (super)interfaces
>>>
>>>             webrev: http://cr.openjdk.java.net/~ronsh/8228675/webrev.00/
>>>             bug: https://bugs.openjdk.java.net/browse/JDK-8228675
>>>
>>>             Thanks,
>>>             Ron
>>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/compiler-dev/attachments/20190807/e5925b18/attachment-0001.html>