RFR 8178150: Regression in logic for handling inference stuck constraints

[Maurizio Cimadamore]

Btw - a possible low hanging fruit performance wise here is to cache the 
closure of a node in the node itself. But this adds complexity as there 
are methods to update dependencies every time the inference graph is 
updated. So the cache would have to be cleared, at the very least in the 
Node::graphChanged method.

Since, as mentioned in the original message, it is likely that we will 
address residual performance concerns in a followup issue, I decided to 
leave this alone - in case we want to come up with a more holistic 
optimization.

Cheers
Maurizio


On 19/10/17 12:08, Maurizio Cimadamore wrote:
> Thanks - I'll address this (and maybe other comments). You are right 
> that a simple for loop is probably more readable here (and better as 
> it avoids stream creation).
>
> I will upload an updated webrev once I accumulate enough comments :-)
>
> Maurizio
>
>
> On 19/10/17 12:04, Andrej Golovnin wrote:
>> Hi Maurizio,
>>
>> in src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Infer.java
>>
>> 1768                         deps.stream().forEach(d ->
>> d.closureInternal(closure));
>>
>> This can be replaced by a simple for-each loop. And if you really
>> don't want to write a for-each loop, then you can can use the
>> forEach-method defined in the Iterable interface:
>>
>> deps.forEach(d -> d.closureInternal(closure));
>>
>> and avoid creation of a stream.
>>
>> Best regards
>> Andrej Golovnin
>>
>> On Thu, Oct 19, 2017 at 11:41 AM, Maurizio Cimadamore
>> <maurizio.cimadamore at oracle.com> wrote:
>>> Updated webrev
>>>
>>> http://cr.openjdk.java.net/~mcimadamore/8178150_v2/
>>>
>>> Maurizio
>>>
>>>
>>> On 19/10/17 09:38, Maurizio Cimadamore wrote:
>>>> I think you are right - this method was there when I first wrote 
>>>> the patch
>>>> but then got removed. I resurrected it for the purpose of this 
>>>> patch, but it
>>>> seems like it's not doing what we need.
>>>>
>>>> Maurizio
>>>>
>>>>
>>>> On 19/10/17 00:57, Vicente Romero wrote:
>>>>> Hi Maurizio,
>>>>>
>>>>> I'm not sure about the correctness or the objective of the closure()
>>>>> method. The termination condition seems to be: stop as soon as you 
>>>>> find a
>>>>> cycle in the graph, at least this is my reading. But for some 
>>>>> graphs this
>>>>> could imply finding a subset of the closure not the whole of it. 
>>>>> It seems to
>>>>> me like implementing a graph traversal method should be what it is 
>>>>> wanted
>>>>> here. Unless there is something I'm missing.
>>>>>
>>>>> Also in the same method the javadoc could have stall comments as 
>>>>> it says
>>>>> that a kind of dependencies is given but there is no argument 
>>>>> passed to the
>>>>> method.
>>>>>
>>>>> Thanks,
>>>>> Vicente
>>>>>
>>>>> On 10/18/2017 04:15 PM, Maurizio Cimadamore wrote:
>>>>>> Hi,
>>>>>> this issue has already been discussed in [1].
>>>>>>
>>>>>> The issue has to do with the fact that logic for picking a 
>>>>>> deferred node
>>>>>> to unstick ignores the can-influence relationship (aka inference 
>>>>>> graph
>>>>>> dependencies).
>>>>>>
>>>>>> The implemented code is not optimized (almost deliberately); I 
>>>>>> wanted
>>>>>> the code to follow the spec more or less cleanly. While I'm open 
>>>>>> to small
>>>>>> improvements, I would please ask to focus on correctness for the 
>>>>>> time being
>>>>>> since (i) this code is not an hot execution path and (ii) other 
>>>>>> performance
>>>>>> improvements in this area will follow, see [2].
>>>>>>
>>>>>> http://cr.openjdk.java.net/~mcimadamore/8178150/
>>>>>>
>>>>>> [1] -
>>>>>> http://mail.openjdk.java.net/pipermail/compiler-dev/2017-October/011192.html 
>>>>>>
>>>>>> [2] -
>>>>>> http://mail.openjdk.java.net/pipermail/compiler-dev/2017-October/011126.html 
>>>>>>
>>>>>>
>>>>>> Cheers
>>>>>> Maurizio
>>>>>>
>