[OpenJDK 2D-Dev] [9] Review Request: 8028486 java/awt/Window/WindowsLeak/WindowsLeak.java fails

[Philip Race]

"incidental" means "not significant".

If this cached object saved a small amount of work in 5% of cases
when in 95% of cases we need to do that work anyway, then
the cache becomes incidental ...

-phil.

On 5/4/16, 6:51 AM, Sergey Bylokhov wrote:
> On 03.05.16 6:37, Philip Race wrote:
>> On 5/2/16, 2:28 PM, Sergey Bylokhov wrote:
>>> On 03.05.16 0:07, Phil Race wrote:
>>>>> And this is fine because we use only identity checks inside the
>>>>> validate method. If the java object was deleted we should update the
>>>>> native state anyway.
>>>>
>>>> Not sure how to read that. Do you mean there is always
>>>> some amount of work to do anyway ?
>>>
>>> An example: if we started the paint operation for some surface and set
>>> the clip, then for the next paint operation we skip setting the
>>> surface and clip if they "==" to the previous values. So if the old
>>> data were deleted by GC will mean that we cannot draw to them(and
>>> these cached references are not useful)
>>>
>>
>> So that is a yes .. this is an incidental amount of work ?
>
> I am not sure what you mean by "incidental amount of work"? Can you 
> please clarify.
>
>>
>>>>
>>>> Is there any value in doing the "nulling" in Anton's earlier version
>>>> of the fix to be more prompt ?
>>>
>>> The current change is one step-up in the chain of references, and the
>>> nulling in the CGlayer code will leave the leak of surface.
>>
>>
>> I know it is not sufficient, but is it a worthwhile addition ?
>
> If we will set some data to null, then we will need to update all 
> usage of it to be ready to null. I think that the current solution 
> cover the problem (but there are some other leaks which will be fixed 
> separately).
>
>>>
>>>> is BufferedContext only used by D3D & OGL ?
>>>
>>> Yes, but I checked direct usage.
>>
>> What does direct usage mean ?
>> The test program does not do this.
>
> I meant that, I grep all usages of this class, and all of these usages 
> in d3d/ogl.
>
>> Sounds good.
>
> I complete full jck/regression testrun(win+d3d), no new bugs were found.
>
>>
>> -phil.
>>
>>>
>>>>>> On 05/02/2016 01:26 PM, Sergey Bylokhov wrote:
>>>>>>> Hello,
>>>>>>> Please review the fix for jdk9.
>>>>>>>
>>>>>>> Bug evaluation was done by Anton:
>>>>>>> http://mail.openjdk.java.net/pipermail/awt-dev/2016-April/011177.html 
>>>>>>>
>>>>>>>
>>>>>>> This is a cross-platform bug it affects d3d/ogl pipelines. The
>>>>>>> problem
>>>>>>> is that BufferedContex cached information to skip some native
>>>>>>> reconfigurations. But this cache cause a memory leak if some
>>>>>>> data(src/dst surfaces) was cached and there was no new rendering in
>>>>>>> this context(we create context per-d3d_device/ogl_config).
>>>>>>>
>>>>>>> In the fix I changed all these caches to weak references. Note 
>>>>>>> that i
>>>>>>> use a references as initial values instead of null, just to 
>>>>>>> eliminate
>>>>>>> the null checks in the body of the method.
>>>>>>>
>>>>>>> The test was updated to be more stable(flushed the EDT + flushed 
>>>>>>> the
>>>>>>> Disposer thread).
>>>>>>>
>>>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8028486
>>>>>>> Webrev can be found at:
>>>>>>> http://cr.openjdk.java.net/~serb/8028486/webrev.00
>>>>>>>
>>>>>>
>>>>>
>>>>>
>>>>
>>>
>>>
>
>