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

[Ron Shapiro]

Happy to give that a try.

On Fri, Aug 9, 2019 at 2:38 PM Maurizio Cimadamore <
maurizio.cimadamore at oracle.com> wrote:

>
> 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.
>
> Maurizio
>
>
>
> 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/63e702db/attachment.html>