<html><head><meta http-equiv="Content-Type" content="text/html charset=utf-8"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class="">Hi Sergey,<div class=""><br class=""></div><div class="">I’m fine with the fix.</div><div class=""><br class=""><div><blockquote type="cite" class=""><div class="">On 12 May 2016, at 14:43, Sergey Bylokhov <<a href="mailto:Sergey.Bylokhov@oracle.com" class="">Sergey.Bylokhov@oracle.com</a>> wrote:</div><br class="Apple-interchange-newline"><div class="">Hi, Anton, Phil.<br class="">Do you have some comments about the current version or it can be considered as an approved?<br class=""><br class="">On 04.05.16 18:06, Philip Race wrote:<br class=""><blockquote type="cite" class="">I recall Anton's fix included a couple of unrelated JNI refs that were not<br class="">being deleted. A separate bug ID would be good for that as it really<br class="">has nothing to do with this problem at all.<br class=""></blockquote></div></blockquote><div><br class=""></div><div><br class=""></div><div>I’d filed it (on review): <a href="https://bugs.openjdk.java.net/browse/JDK-8156116" class="">https://bugs.openjdk.java.net/browse/JDK-8156116</a></div><div><br class=""></div><div>Regards,</div><div>Anton.</div><br class=""><blockquote type="cite" class=""><div class=""><blockquote type="cite" class=""><br class="">-phil.<br class=""><br class="">On 5/4/16, 7:08 AM, Sergey Bylokhov wrote:<br class=""><blockquote type="cite" class="">On 04.05.16 16:07, Anton Tarasov wrote:<br class=""><blockquote type="cite" class="">Hi Sergey,<br class=""><br class="">As you fixed the surface leak separately (thanks!) should I create a<br class="">dedicated CR for the unrelated JNI ones?<br class=""></blockquote><br class="">Can you please clarify this case? Do you have a testcase or steps to<br class="">reproduce?(WindowsLeak.java will pass after the current fix).<br class=""><br class=""><br class=""><blockquote type="cite" class=""><blockquote type="cite" class="">On 29 Apr 2016, at 18:19, Sergey Bylokhov<br class=""><<a href="mailto:Sergey.Bylokhov@oracle.com" class="">Sergey.Bylokhov@oracle.com</a>> wrote:<br class=""><br class="">On 29.04.16 18:01, Anton Tarasov wrote:<br class=""><blockquote type="cite" class="">[CC’ing to 2d-dev to discuss the issue]<br class=""><br class=""><blockquote type="cite" class="">On 29 Apr 2016, at 16:14, Sergey Bylokhov<br class=""><<a href="mailto:Sergey.Bylokhov@oracle.com" class="">Sergey.Bylokhov@oracle.com</a>> wrote:<br class=""><br class="">On 29.04.16 15:53, Anton Tarasov wrote:<br class=""><blockquote type="cite" class="">It seems so. But that might be not that critical, because it<br class="">doesn’t hold (it won’t) any UI controls and all the UI tree.<br class="">Anyway it leaks, yes.<br class=""></blockquote><br class="">Looks like this is crossplatform bug and it also affects d3d/ogl.<br class="">So probably we can fix it on the upper level?<br class="">validatedSrc/Dst/Data is stored the surfaces which were validated<br class="">and ready to paint. from the first point of view we can change<br class="">them to soft reference, and take care about null values.<br class=""></blockquote><br class="">Are the validatedSrc/Dst/Data objects referenced from somewhere<br class="">else? They are private, so from native? If not, soft refs won’t<br class="">help I’m afraid...<br class=""></blockquote><br class="">I guess it is used in BufferedContext only, to skip updates of the<br class="">native ogl/d3d context if the target/source surfaces were not<br class="">changed since the last update.<br class=""><br class=""><blockquote type="cite" class=""><br class="">This is not the case for CGLLayer, which is referenced from JNI.<br class="">And so, wrapping it with a weak ref will work. Also, if the<br class="">SurfaceData is uniquely tight to the layer, then it seems natural<br class="">to dispose (not flush) it with the layer disposal. And that’s<br class="">actually the case: LWWindowPeer.disposeImpl() invalidates it.<br class="">But the problem is that invalidation doesn’t release the layer.<br class=""></blockquote><br class="">Yes, that's right the surface and layer are bound to each other(and<br class="">the layer can have more than one surface). So I do not see a reason<br class="">why we should break the link between them, which causes the surface<br class="">to live more time than its layer. I guess the right things to do is<br class="">to fix the "gc root", since we have no cycles here.<br class=""><br class=""><blockquote type="cite" class=""><br class="">So, again the question is: should the layer be nullified on<br class="">invalidation or it should be made a weak ref? For me this seems<br class="">quite logical to release resources on disposal/invalidation, what<br class="">do you think?<br class=""><br class="">As to the fixing the issue globally, I don’t have enough<br class="">understanding of the pipe design so that to do that properly. For<br class="">instance, as I wrote before, I don’t know under which conditions<br class="">the context should/may be disposed…<br class=""><br class="">May be someone else can advice on it.<br class=""></blockquote><br class="">I can take a look, but are you sure that the test "WindowsLeak.java"<br class="">reproduce exactly your problem?<br class=""><br class=""><blockquote type="cite" class=""><blockquote type="cite" class="">Note that such changes are 2d related code and should be reviewed<br class="">on 2d-dev.<br class=""><br class=""><blockquote type="cite" class=""><br class=""><blockquote type="cite" class="">Assigned the peer/surfaceData to null in CGLayer can causes an<br class="">NPE in all its usage, because there is no any synchronization<br class="">which will prevent the usage of CGLayer after disposing.<br class=""></blockquote><br class="">That’s bad. Will wrapping the refs with AtomicReference help?<br class=""><br class=""><blockquote type="cite" class=""><br class="">Unrelated to the fix, but it seems we should call flush() on<br class="">surface when the layer is disposed, at least I do not understand<br class="">where we flush the native ogl data for the latest surface data.<br class=""></blockquote><br class=""><br class="">This will trigger CGLLayerSurfaceData.invalidate(), but the<br class="">“layer” will still not be nullified. What about nullifying it in<br class="">invalidate()? Will we face the same synchronisation issue?<br class=""><br class="">Anton.<br class=""><br class=""><blockquote type="cite" class=""><br class="">On 29.04.16 15:00, Anton Tarasov wrote:<br class=""><blockquote type="cite" class="">Hi Sergey, Alexander,<br class=""><br class="">Please review the fix:<br class=""><br class="">bug: JDK-8028486 [TEST_BUG] [macosx]<br class="">java/awt/Window/WindowsLeak/WindowsLeak.java fails<br class="">webrev: <a href="http://cr.openjdk.java.net/~ant/JDK-8028486/webrev.0" class="">http://cr.openjdk.java.net/~ant/JDK-8028486/webrev.0</a><br class=""><br class="">I’m copying my comment from CR:<br class=""><br class="">Please open the attached screenshot [*], made with YourKit,<br class="">where a chain of links is shown from the GC roots.<br class="">The frame is held by its peer which is held by CGLLayer which<br class="">is held as validatedSrcData in the GL context.<br class="">The point is that the GL context doesn't cleanup the last state<br class="">until under some conditions, which are not applicable to this<br class="">scenario.<br class="">I'm not sure should the cleanup be triggered here or not, but<br class="">the problem can be solved otherwise.<br class=""><br class="">The point is that in the chain the CGLLayer instance has been<br class="">disposed, in response to the frame disposal.<br class="">So, this is the only ref that holds it (the JNI ref is released<br class="">by the native peer on disposal).<br class="">Thus, as the layer is disposed it can at least zero all the<br class="">java refs it holds (this change already fixes the problem).<br class="">Then, the "layer" ref in CGLLayerSurfaceData should probably be<br class="">made weak.<br class=""><br class="">[*]<br class=""><a href="https://bugs.openjdk.java.net/secure/attachment/59121/8028486.png" class="">https://bugs.openjdk.java.net/secure/attachment/59121/8028486.png</a><br class=""><br class="">As to the “weak ref” mentioned in the comment. I didn’t do<br class="">that, but if you find it reasonable I can add that change (or<br class="">file a separate CR).<br class=""><br class="">Also, the fix contains some additional cleanup (not related to<br class="">this CR): two more JNI local refs leak, fixed.<br class=""><br class="">Thanks,<br class="">Anton.<br class=""><br class=""></blockquote><br class=""><br class="">--<br class="">Best regards, Sergey.<br class=""></blockquote><br class=""></blockquote><br class=""><br class="">--<br class="">Best regards, Sergey.<br class=""></blockquote><br class=""></blockquote><br class=""><br class="">--<br class="">Best regards, Sergey.<br class=""></blockquote><br class=""></blockquote><br class=""><br class=""></blockquote></blockquote><br class=""><br class="">-- <br class="">Best regards, Sergey.<br class=""></div></blockquote></div><br class=""></div></body></html>