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

Fri Aug 9 11:08:40 UTC 2019

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.

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> 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>
> 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 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>:
>>
>>> 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
>>> >:
>>>
>>>> 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/218d9935/attachment.html>