Hi Sergey, I’m fine with the fix.
> On 12 May 2016, at 14:43, Sergey Bylokhov <sergey.bylok...@oracle.com> wrote: > > Hi, Anton, Phil. > Do you have some comments about the current version or it can be considered > as an approved? > > On 04.05.16 18:06, Philip Race wrote: >> I recall Anton's fix included a couple of unrelated JNI refs that were not >> being deleted. A separate bug ID would be good for that as it really >> has nothing to do with this problem at all. I’d filed it (on review): https://bugs.openjdk.java.net/browse/JDK-8156116 <https://bugs.openjdk.java.net/browse/JDK-8156116> Regards, Anton. >> >> -phil. >> >> On 5/4/16, 7:08 AM, Sergey Bylokhov wrote: >>> On 04.05.16 16:07, Anton Tarasov wrote: >>>> Hi Sergey, >>>> >>>> As you fixed the surface leak separately (thanks!) should I create a >>>> dedicated CR for the unrelated JNI ones? >>> >>> Can you please clarify this case? Do you have a testcase or steps to >>> reproduce?(WindowsLeak.java will pass after the current fix). >>> >>> >>>>> 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. >>>>> >>>>>> >>>>>> 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. >>>>> >>>>>> >>>>>> 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? >>>>> >>>>>>> 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. >>>> >>> >>> > > > -- > Best regards, Sergey.
