> On 29 Apr 2016, at 18:19, Sergey Bylokhov <sergey.bylok...@oracle.com> wrote: > > On 29.04.16 18:01, Anton Tarasov wrote: >> [CC’ing to 2d-dev to discuss the issue] >> >>> On 29 Apr 2016, at 16:14, Sergey Bylokhov <sergey.bylok...@oracle.com> >>> wrote: >>> >>> On 29.04.16 15:53, Anton Tarasov wrote: >>>> It seems so. But that might be not that critical, because it doesn’t hold >>>> (it won’t) any UI controls and all the UI tree. Anyway it leaks, yes. >>> >>> Looks like this is crossplatform bug and it also affects d3d/ogl. So >>> probably we can fix it on the upper level? validatedSrc/Dst/Data is stored >>> the surfaces which were validated and ready to paint. from the first point >>> of view we can change them to soft reference, and take care about null >>> values. >> >> Are the validatedSrc/Dst/Data objects referenced from somewhere else? They >> are private, so from native? If not, soft refs won’t help I’m afraid... > > I guess it is used in BufferedContext only, to skip updates of the native > ogl/d3d context if the target/source surfaces were not changed since the last > update.
Ok. > >> >> This is not the case for CGLLayer, which is referenced from JNI. And so, >> wrapping it with a weak ref will work. Also, if the SurfaceData is uniquely >> tight to the layer, then it seems natural to dispose (not flush) it with the >> layer disposal. And that’s actually the case: LWWindowPeer.disposeImpl() >> invalidates it. >> But the problem is that invalidation doesn’t release the layer. > > Yes, that's right the surface and layer are bound to each other(and the layer > can have more than one surface). So I do not see a reason why we should break > the link between them, which causes the surface to live more time than its > layer. I guess the right things to do is to fix the "gc root", since we have > no cycles here. Are you suggesting to kill GraphicsEnvironment? But what if frames are recreated? Then recreate it? > >> >> So, again the question is: should the layer be nullified on invalidation or >> it should be made a weak ref? For me this seems quite logical to release >> resources on disposal/invalidation, what do you think? >> >> As to the fixing the issue globally, I don’t have enough understanding of >> the pipe design so that to do that properly. For instance, as I wrote >> before, I don’t know under which conditions the context should/may be >> disposed… >> >> May be someone else can advice on it. > > I can take a look, but are you sure that the test "WindowsLeak.java" > reproduce exactly your problem? My problem is that there’s a number of revealed JDK leaks on Mac. And this one looks like the last in the list ) So, if you please take a look I’d appreciate. Thanks, Anton. > >>> Note that such changes are 2d related code and should be reviewed on 2d-dev. >>> >>>> >>>>> Assigned the peer/surfaceData to null in CGLayer can causes an NPE in all >>>>> its usage, because there is no any synchronization which will prevent the >>>>> usage of CGLayer after disposing. >>>> >>>> That’s bad. Will wrapping the refs with AtomicReference help? >>>> >>>>> >>>>> Unrelated to the fix, but it seems we should call flush() on surface when >>>>> the layer is disposed, at least I do not understand where we flush the >>>>> native ogl data for the latest surface data. >>>> >>>> >>>> This will trigger CGLLayerSurfaceData.invalidate(), but the “layer” will >>>> still not be nullified. What about nullifying it in invalidate()? Will we >>>> face the same synchronisation issue? >>>> >>>> Anton. >>>> >>>>> >>>>> On 29.04.16 15:00, Anton Tarasov wrote: >>>>>> Hi Sergey, Alexander, >>>>>> >>>>>> Please review the fix: >>>>>> >>>>>> bug: JDK-8028486 [TEST_BUG] [macosx] >>>>>> java/awt/Window/WindowsLeak/WindowsLeak.java fails >>>>>> webrev: http://cr.openjdk.java.net/~ant/JDK-8028486/webrev.0 >>>>>> >>>>>> I’m copying my comment from CR: >>>>>> >>>>>> Please open the attached screenshot [*], made with YourKit, where a >>>>>> chain of links is shown from the GC roots. >>>>>> The frame is held by its peer which is held by CGLLayer which is held as >>>>>> validatedSrcData in the GL context. >>>>>> The point is that the GL context doesn't cleanup the last state until >>>>>> under some conditions, which are not applicable to this scenario. >>>>>> I'm not sure should the cleanup be triggered here or not, but the >>>>>> problem can be solved otherwise. >>>>>> >>>>>> The point is that in the chain the CGLLayer instance has been disposed, >>>>>> in response to the frame disposal. >>>>>> So, this is the only ref that holds it (the JNI ref is released by the >>>>>> native peer on disposal). >>>>>> Thus, as the layer is disposed it can at least zero all the java refs it >>>>>> holds (this change already fixes the problem). >>>>>> Then, the "layer" ref in CGLLayerSurfaceData should probably be made >>>>>> weak. >>>>>> >>>>>> [*] https://bugs.openjdk.java.net/secure/attachment/59121/8028486.png >>>>>> >>>>>> As to the “weak ref” mentioned in the comment. I didn’t do that, but if >>>>>> you find it reasonable I can add that change (or file a separate CR). >>>>>> >>>>>> Also, the fix contains some additional cleanup (not related to this CR): >>>>>> two more JNI local refs leak, fixed. >>>>>> >>>>>> Thanks, >>>>>> Anton. >>>>>> >>>>> >>>>> >>>>> -- >>>>> Best regards, Sergey. >>>> >>> >>> >>> -- >>> Best regards, Sergey. >> > > > -- > Best regards, Sergey.
