RFR 8178150: Regression in logic for handling inference stuck constraints

[Vicente Romero]

Hi Maurizio,

Thanks for the updated version. I agree with you that once the closure 
of a given node is calculated it should be cached. Probably in an 
independent data structure as it could be expensive to calculate the 
closure and also because for a given graph, the closure will be the same 
for a given node until the graph changes. I still have comments on the 
performance land but I would like to focus on the correctness first. 
This code, at DeferredAttrContext::buildStuckGraph:

InferenceGraph graph = infer.new GraphSolver(inferenceContext, 
types.noWarnings)
                     .new InferenceGraph();

will return a graph in which strongly connected components have been 
merged along with theirs dependencies, meaning that if variable T1 and 
T2 are strongly connected, dependencies of T1 will be the same as 
dependencies of T2. I'm not sure that the spec considered this graph as 
the one over which to calculate input and output variables. Also I'm 
wondering if it could be the case that the tarjan algorithm could place 
an input and an output variable in the same node.

Apart from this there are assumptions that even though are previous to 
this code, could affect the outcome. It is been assumed that stuck 
variables == input variables. But according to the spec in this case

‹LambdaExpression → throws T ›

T is an input variable, but I don't think that it will be considered as 
a stuck variable. Similar for method references.

Thanks,
Vicente

On 10/19/2017 05:41 AM, Maurizio Cimadamore 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
>>>>
>>>
>>
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/compiler-dev/attachments/20171024/14178f37/attachment.html>