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

Maurizio Cimadamore maurizio.cimadamore at oracle.com
Fri Aug 9 11:38:21 UTC 2019

On 09/08/2019 12:08, Ron Shapiro wrote:
> I don't actually understand what any of those code is doing, so I 
> don't think I really have much _preference_. But if I understand 
> correctly, it does seem like what you propose would not suffice should 
> there be at least one default method in the hierarchy.
> In our specific case I don't believe there is, but it seems like 
> possibly a more narrow optimization.

Sure, it is more narrow - but I think this optimization is really only 
needed when there are lots of auto-generated files, in which case it's 
very likely (I'm assuming) that plain interfaces (w/o default methods) 
will be used; it is also a less risky optimization, with less chances 
for introducing accidental memory footprint regression in a core area of 
the compiler, so I'd be more confident in starting there, and see where 
it leads us.


> If you have a patch for what you're suggesting, I can apply it and see 
> if it does at least have the same effects as the above webrevs
> On Thu, Aug 8, 2019 at 12:04 AM Maurizio Cimadamore 
> <maurizio.cimadamore at oracle.com 
> <mailto:maurizio.cimadamore at oracle.com>> wrote:
>     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/20190809/80d0af1d/attachment.html>

More information about the compiler-dev mailing list