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

[Brad Corso]

> 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/?

On Fri, Sep 6, 2019 at 4:08 AM Maurizio Cimadamore <
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> 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> 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>> 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/20190917/f750ec8f/attachment-0001.html>