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

[Sergey Bylokhov]

On 04.05.16 18:08, Philip Race wrote:
> "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 ...

It actually "depends on": if the user will paint to the different 
VolatileImages/Windows in parallel, then this cache will be useless(we 
will updates the state on each call, because OGL/D3D pipelines are 
single-threaded), but if the user will paint lots of stuff to one 
VolatileImage, then we will save some time because the native state will 
be updated only once(for example setting the clip can be quite slow 
operation).

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


-- 
Best regards, Sergey.