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