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.
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
> <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