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.
