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

Maurizio Cimadamore maurizio.cimadamore at oracle.com
Thu Sep 19 11:18:17 UTC 2019


On 18/09/2019 02:06, Brad Corso wrote:
> > This patch seems like a low-hanging fruit - union was already 
> returning element in that order in case they were unrelated, so I 
> agree the extra test looks redundant.
>
> Thanks for taking a look Maurizio.
>
> What's the best way to get this submitted? Should we file a separate 
> bug for the changes in 
> http://cr.openjdk.java.net/~ronsh/8228675/webrev.02/ 
> <http://cr.openjdk.java.net/~ronsh/8228675/webrev.02/>?
>
I think that would be best, yes

Maurizio

> On Fri, Sep 6, 2019 at 4:08 AM Maurizio Cimadamore 
> <maurizio.cimadamore at oracle.com 
> <mailto:maurizio.cimadamore at oracle.com>> wrote:
>
>
>     On 06/09/2019 11:44, Maurizio Cimadamore wrote:
>>
>>     For the records, I did some internal performance tests, and this
>>     patch doesn't seem to make the performance situation worse in the
>>     general case.
>>
>>     I'm now curious to see if it helps with your specific use case.
>>
>     More generally speaking, for this - and others - performance
>     related issues, we're happy to look into them, but I think it is
>     generally good to submit a reproducible test case that we can use
>     as a playground; this will help us understanding what the problem
>     is, understanding what are the connections to other problems in
>     the code base, as well as to assess its priority. Such a test case
>     would also form the backbone of any test that would be pushed
>     upstream (as it is generally preferrable to always include tests
>     in patches). I know that sometimes distilling such tests takes
>     time, but without them we're basically throwing darts in the dark.
>
>     Maurizio
>
>
>>     Maurizio
>>
>>     On 04/09/2019 20:58, Maurizio Cimadamore wrote:
>>>
>>>     Hi Ron,
>>>     I put together something, albeit not the approach I described in
>>>     this thread.
>>>
>>>     Since, from what I'm reading, the main issue you are seeing is
>>>     that the use of Types.closure is basically killing performances,
>>>     I addressed that problem instead, and rewrote the code not to
>>>     use that method. The new code is probably executing more checks
>>>     than the old (since interfaces could be implemented multiple
>>>     times at different levels of the hierarchy), but I think it
>>>     should be enough to resolve the issue you are seeing.
>>>
>>>     I did not spend a lot of time testing the patch, other than
>>>     doing some basic validation - so handle with care :-)
>>>
>>>     http://cr.openjdk.java.net/~mcimadamore/8228675/
>>>
>>>     Maurizio
>>>
>>>     On 04/09/2019 15:18, Maurizio Cimadamore wrote:
>>>>
>>>>     I didn't realize you were waiting on me to produce a patch; I'm
>>>>     afraid at the moment I don't have many cycles to look into this.
>>>>
>>>>     Maurizio
>>>>
>>>>     On 04/09/2019 13:05, Ron Shapiro wrote:
>>>>>     Friendly ping. This is really hurting some of our builds.
>>>>>     Maurizio, are you able to prepare a patch for us to look at?
>>>>>
>>>>>     On Tue, Aug 13, 2019 at 8:12 AM Ron Shapiro
>>>>>     <ronshapiro at google.com <mailto:ronshapiro at google.com>> wrote:
>>>>>
>>>>>         Aren't types recreated across rounds if there are any
>>>>>         transitive error types? That was at least my assumption.
>>>>>
>>>>>         On Mon, Aug 12, 2019 at 5:50 PM Jan Lahoda
>>>>>         <jan.lahoda at oracle.com <mailto:jan.lahoda at oracle.com>> wrote:
>>>>>
>>>>>             I wonder if this will work with annotation processors?
>>>>>             (As these may
>>>>>             create new interfaces into the hierarchy.)
>>>>>
>>>>>             Jan
>>>>>
>>>>>             On 07. 08. 19 12: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>
>>>>>             > <mailto: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>
>>>>>             <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>
>>>>>             <mailto: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>
>>>>>             <mailto: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/20190919/49760ac8/attachment-0001.html>


More information about the compiler-dev mailing list