RFR 8221118: Avoid eagerly creating JCDiagnostic for CompletionFailures

[Ron Shapiro]

>From what I saw, JCDiagnostic.normalize() was taking the bulk of the time
(I think ~80%). That was all spent in Arrays.stream()....toArray(), which I
presume was creating a lot of garbage. Inserting a loop to first check if
any normalization was necessary removed that chunk, but I was still seeing
a sizeable remainder from other things (perhaps the two methods you listed).

I can try to dig up the initial profiles, but most of these builds tend to
be in the 100s-120s range.

>From what the code is currently doing, `() -> null` could be made into just
`null`. On my own, I didn't feel comfortable introducing that and preferred
to remain consistent, but if you think I should make the change I defer to
you.



On Thu, Mar 28, 2019 at 7:23 AM Maurizio Cimadamore <
maurizio.cimadamore at oracle.com> wrote:

> Looks mostly good - as Jon mentioned, eager diagnostic generation
> sometimes bites back. In the cases we have seen in the past, the main issue
> was related to the fact that, in order to create the diagnostic, the client
> had to call expensive methods (such as TreeInfo::diagnosticPositionFor, as
> per JDK-8215368). In this case, I see two somewhat potentially expensive
> operations:
>
> * ex.getLocalizedMessage()
>
> and
>
> * createBadClassFileDiagnostic
>
> Although neither seem to be too bad (e.g. requiring visitors, loops, or
> whatever). So, while in principle I agree with the change, I'm somewhat
> skeptical that it buys much in terms of performances. I've seen your bug
> report mentioned 700ms up to 3s spent in diagnostic creation... out of what
> total execution time?
>
> Also, the () -> null looks a tad suspicious. If we don't care about the
> diagnostic, do we really want to create a new lambda for it? (I would
> normally not care much about this kind of micro optimization, but since
> that seems the whole point of this webrev, I thought it was worth pointing
> out).
>
> Maurizio
> On 27/03/2019 17:46, Ron Shapiro wrote:
>
> I missed a few cases earlier - see this updated webrev:
> http://cr.openjdk.java.net/~ronsh/8221118/webrev.01/
>
> On Tue, Mar 26, 2019 at 4:10 PM Jonathan Gibbons <
> jonathan.gibbons at oracle.com> wrote:
>
>> Looks OK to me, especially the similarity to JDK-8215368
>>
>> -- Jon
>>
>> On 03/19/2019 03:37 PM, Ron Shapiro wrote:
>>
>> Hi,
>>
>> This is a small change that avoids eagerly creating JCDiagnostic for
>> CompletionFailures.
>>
>> webrev: http://cr.openjdk.java.net/~ronsh/8221118/webrev.00/
>> bug: https://bugs.openjdk.java.net/browse/JDK-8221118
>>
>> Thanks,
>> Ron
>>
>>
>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/compiler-dev/attachments/20190328/c58889e5/attachment.html>