[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-openjfx-dev
Subject: Re: [External] : Re: New API: Animation/Timeline improvement
From: Andy Goryachev <andy.goryachev () oracle ! com>
Date: 2023-12-19 17:56:20
Message-ID: DM5PR1001MB21727218566433F6F4FA1A8AE597A () DM5PR1001MB2172 ! namprd10 ! prod ! outlook ! com
[Download RAW message or body]
You are right: the weak references are not suitable in this case.
These cases are clearly bugs in the skins, we should log them (unless already \
logged); I don’t think we need new APIs.
See
https://bugs.openjdk.org/issues/?jql=text%20~%20%22skin%20cleanup%22%20AND%20project%2 \
0%3D%20JDK%20AND%20component%20%3D%20javafx%20AND%20resolution%20%3D%20Unresolved%20ORDER%20BY%20updated%20%20DESC
There is an umbrella task
https://bugs.openjdk.org/browse/JDK-8241364
which collects all these issues.
Care to create JBS tickets?
Thanks!
-andy
From: John Hendrikx <john.hendrikx@gmail.com>
Date: Monday, December 18, 2023 at 16:14
To: Andy Goryachev <andy.goryachev@oracle.com>, openjfx-dev <openjfx-dev@openjdk.org>
Subject: [External] : Re: New API: Animation/Timeline improvement
I don't think that's the correct solution, as it opens up a whole new avenue for \
subtle bugs, bringing GC/JVM/OS whims and settings into the mix. We want the clean-up \
to be easier to reason about, not harder.
Even if it were a good idea, it's likely also going to be a breaking change to add \
weak references at this stage, without some kind of opt-in (which would then require \
new API, in which case we might as well go for a solution that has no need of weak \
references).
I feel I have to keep repeating this, but there almost zero guarantees made by the \
JVM or Java with regards to references; they are fine for internal processes \
carefully designed to have no user visible side effects, but burdening the user with \
side effects (delayed clean-up, or early unexpected clean-up due to lack of a hard \
reference) without the user actually choosing to use a reference type themselves is \
not a good design.
--John
On 18/12/2023 17:18, Andy Goryachev wrote:
Would making Timeline to use WeakReferences solve the issue without the need for a \
new API?
-andy
From: openjfx-dev <openjfx-dev-retn@openjdk.org><mailto:openjfx-dev-retn@openjdk.org> \
on behalf of John Hendrikx \
<john.hendrikx@gmail.com><mailto:john.hendrikx@gmail.com>
Date: Friday, December 15, 2023 at 21:53
To: openjfx-dev <openjfx-dev@openjdk.org><mailto:openjfx-dev@openjdk.org>
Subject: New API: Animation/Timeline improvement
Hi list,
I've noticed that Animations and Timelines are often a source of leaks,
and their clean-up is often either non-existent or incorrect. The
reason they cause leaks easily is because a running animation or
timeline is globally referred from a singleton PrimaryTimer. The
animation or timeline then refers to properties or event handlers which
refer to controls (which refer to parents and the entire scene).
For example:
- ScrollBarBehavior uses a Timeline, but neglects to clean it up. If it
was running at the time a Scene is detached from a Window, and that
Scene is left to go out of scope, it won't because Timeline refers it;
this can happen if the behavior never receives a key released event.
- ScrollBarBehavior has no dispose method overridden, so swapping Skins
while the animation is running will leave a Timeline active (it uses
Animation.INDEFINITE)
- SpinnerBehavior has flawed clean up; it attaches a Scene listener and
disables its timeline when the scene changed, but the scene doesn't have
to change for it to go out of scope as a whole... Result is that if you
have a spinner timeline running, and you close the entire window (no
Scene change happens), the entire Scene will still be referred. It also
uses an indefinite cycle count. It also lacks a dispose method, so
swapping Skins at a bad moment can also leave a reference.
I think these mistakes are common, and far too easy to make. The most
common use cases for animations revolve around modifying properties on
visible controls, and although animations can be used for purposes other
than animating UI controls, this is extremely rare. So it is safe to
say that in 99% of cases you want the animation to stop once a some Node
is no longer showing. For both the mentioned buggy behaviors above, this
would be perfect. A spinner stops spinning when no longer showing, and
a scroll bar stops scrolling when no longer showing. It is also likely
to apply for many other uses of timelines and animations.
I therefore want to propose a new API, either on Node or Animation (or
both):
/**
* Creates a new timeline which is stopped automatically when this Node
* is no longer showing. Stopping timelines is essential as they
may refer
* nodes even after they are no longer used anywhere, preventing
them from
* being garbage collected.
*/
Node.createTimeline(); // and variants with the various Timeline
constructors
And/or:
/**
* Links this Animation to the given Node, and stops the animation
* automatically when the Node is no longer showing. Stopping
animations
* is essential as they may refer nodes even after they are no
longer used
* anywhere, preventing them from being garbage collected.
*/
void stopWhenHidden(Node node);
The above API for Animation could also be provided through another
constructor, which takes a Node which will it be linked to.
Alternatives:
- Be a lot more diligent about cleaning up animations and timelines
(essentially maintain the status quo which has led to above bugs)
- Use this lengthy code fragment below:
Timeline timeline = new Timeline();
someNode.sceneProperty()
.when(timeline.statusProperty().map(status -> status !=
Status.STOPPED))
.flatMap(Scene::windowProperty)
.flatMap(Window::showingProperty)
.orElse(false)
.subscribe(showing -> {
if (!showing) timeline.stop();
});
The `when` line ensures that the opposite problem (Nodes forever
referencing Timelines) doesn't occur if you are creating a new Timeline
for each use (not recommended, but nonetheless a common occurrence).
--John
[Attachment #3 (text/html)]
<html xmlns:o="urn:schemas-microsoft-com:office:office" \
xmlns:w="urn:schemas-microsoft-com:office:word" \
xmlns:m="http://schemas.microsoft.com/office/2004/12/omml" \
xmlns="http://www.w3.org/TR/REC-html40"> <head>
<meta http-equiv="Content-Type" content="text/html; charset=Windows-1252">
<meta name="Generator" content="Microsoft Word 15 (filtered medium)">
<style><!--
/* Font Definitions */
@font-face
{font-family:"Cambria Math";
panose-1:2 4 5 3 5 4 6 3 2 4;}
@font-face
{font-family:"Yu Gothic";
panose-1:2 11 4 0 0 0 0 0 0 0;}
@font-face
{font-family:Calibri;
panose-1:2 15 5 2 2 2 4 3 2 4;}
@font-face
{font-family:Aptos;
panose-1:2 11 0 4 2 2 2 2 2 4;}
@font-face
{font-family:"Iosevka Fixed SS16";
panose-1:2 0 5 9 3 0 0 0 0 4;}
@font-face
{font-family:"Times New Roman \(Body CS\)";
panose-1:2 11 6 4 2 2 2 2 2 4;}
@font-face
{font-family:"\@Yu Gothic";
panose-1:2 11 4 0 0 0 0 0 0 0;}
/* Style Definitions */
p.MsoNormal, li.MsoNormal, div.MsoNormal
{margin:0in;
font-size:10.0pt;
font-family:"Calibri",sans-serif;}
a:link, span.MsoHyperlink
{mso-style-priority:99;
color:blue;
text-decoration:underline;}
span.EmailStyle21
{mso-style-type:personal-reply;
font-family:"Iosevka Fixed SS16";
color:windowtext;}
.MsoChpDefault
{mso-style-type:export-only;
font-size:10.0pt;
mso-ligatures:none;}
@page WordSection1
{size:8.5in 11.0in;
margin:1.0in 1.0in 1.0in 1.0in;}
div.WordSection1
{page:WordSection1;}
--></style>
</head>
<body lang="EN-US" link="blue" vlink="purple" style="word-wrap:break-word">
<div class="WordSection1">
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Iosevka Fixed \
SS16"">You are right: the weak references are not suitable in this case. \
<o:p></o:p></span></p> <p class="MsoNormal"><span \
style="font-size:11.0pt;font-family:"Iosevka Fixed \
SS16""><o:p> </o:p></span></p> <p class="MsoNormal"><span \
style="font-size:11.0pt;font-family:"Iosevka Fixed SS16"">These cases are \
clearly bugs in the skins, we should log them (unless already logged); I don’t think \
we need new APIs.<o:p></o:p></span></p> <p class="MsoNormal"><span \
style="font-size:11.0pt;font-family:"Iosevka Fixed \
SS16""><o:p> </o:p></span></p> <p class="MsoNormal"><span \
style="font-size:11.0pt;font-family:"Iosevka Fixed \
SS16"">See<o:p></o:p></span></p> <p class="MsoNormal"><span \
style="font-size:11.0pt;font-family:"Iosevka Fixed \
SS16""><o:p> </o:p></span></p> <p class="MsoNormal"><span \
style="font-size:11.0pt;font-family:"Iosevka Fixed SS16""><a \
href="https://bugs.openjdk.org/issues/?jql=text%20~%20%22skin%20cleanup%22%20AND%20pro \
ject%20%3D%20JDK%20AND%20component%20%3D%20javafx%20AND%20resolution%20%3D%20Unresolve \
d%20ORDER%20BY%20updated%20%20DESC">https://bugs.openjdk.org/issues/?jql=text%20~%20%2 \
2skin%20cleanup%22%20AND%20project%20%3D%20JDK%20AND%20component%20%3D%20javafx%20AND% \
20resolution%20%3D%20Unresolved%20ORDER%20BY%20updated%20%20DESC</a><o:p></o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Iosevka Fixed \
SS16""><o:p> </o:p></span></p> <p class="MsoNormal"><span \
style="font-size:11.0pt;font-family:"Iosevka Fixed SS16"">There is an \
umbrella task<o:p></o:p></span></p> <p class="MsoNormal"><span \
style="font-size:11.0pt;font-family:"Iosevka Fixed \
SS16""><o:p> </o:p></span></p> <p class="MsoNormal"><span \
style="font-size:11.0pt;font-family:"Iosevka Fixed SS16""><a \
href="https://bugs.openjdk.org/browse/JDK-8241364">https://bugs.openjdk.org/browse/JDK-8241364</a><o:p></o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Iosevka Fixed \
SS16""><o:p> </o:p></span></p> <p class="MsoNormal"><span \
style="font-size:11.0pt;font-family:"Iosevka Fixed SS16"">which collects \
all these issues.<o:p></o:p></span></p> <p class="MsoNormal"><span \
style="font-size:11.0pt;font-family:"Iosevka Fixed \
SS16""><o:p></o:p></span></p> <p class="MsoNormal"><span \
style="font-size:11.0pt;font-family:"Iosevka Fixed SS16"">Care to create \
JBS tickets?<o:p></o:p></span></p> <p class="MsoNormal"><span \
style="font-size:11.0pt;font-family:"Iosevka Fixed \
SS16""><o:p> </o:p></span></p> <p class="MsoNormal"><span \
style="font-size:11.0pt;font-family:"Iosevka Fixed \
SS16"">Thanks!<o:p></o:p></span></p> <p class="MsoNormal"><span \
style="font-size:11.0pt;font-family:"Iosevka Fixed \
SS16"">-andy<o:p></o:p></span></p> <p class="MsoNormal"><span \
style="font-size:11.0pt;font-family:"Iosevka Fixed \
SS16""><o:p> </o:p></span></p> <p class="MsoNormal"><span \
style="font-size:11.0pt;font-family:"Iosevka Fixed \
SS16""><o:p> </o:p></span></p> <p class="MsoNormal"><span \
style="font-size:11.0pt;font-family:"Iosevka Fixed \
SS16""><o:p> </o:p></span></p> <div \
id="mail-editor-reference-message-container"> <div>
<div style="border:none;border-top:solid #B5C4DF 1.0pt;padding:3.0pt 0in 0in 0in">
<p class="MsoNormal" style="margin-bottom:12.0pt"><b><span \
style="font-size:12.0pt;font-family:"Aptos",sans-serif;color:black">From: \
</span></b><span style="font-size:12.0pt;font-family:"Aptos",sans-serif;color:black">John \
Hendrikx <john.hendrikx@gmail.com><br> <b>Date: </b>Monday, December 18, 2023 \
at 16:14<br> <b>To: </b>Andy Goryachev <andy.goryachev@oracle.com>, openjfx-dev \
<openjfx-dev@openjdk.org><br> <b>Subject: </b>[External] : Re: New API: \
Animation/Timeline improvement<o:p></o:p></span></p> </div>
<p>I don't think that's the correct solution, as it opens up a whole new avenue for \
subtle bugs, bringing GC/JVM/OS whims and settings into the mix. We want the clean-up \
to be easier to reason about, not harder. <o:p></o:p></p>
<p>Even if it were a good idea, it's likely also going to be a breaking change to add \
weak references at this stage, without some kind of opt-in (which would then require \
new API, in which case we might as well go for a solution that has no need of weak \
references).<o:p></o:p></p> <p>I feel I have to keep repeating this, but there almost \
zero guarantees made by the JVM or Java with regards to references; they are fine for \
internal processes carefully designed to have no user visible side effects, but \
burdening the user with side effects (delayed clean-up, or early unexpected clean-up \
due to lack of a hard reference) without the user actually choosing to use a \
reference type themselves is not a good design.<o:p></o:p></p> \
<p>--John<o:p></o:p></p> <div>
<p class="MsoNormal"><span style="font-size:11.0pt">On 18/12/2023 17:18, Andy \
Goryachev wrote:<o:p></o:p></span></p> </div>
<blockquote style="margin-top:5.0pt;margin-bottom:5.0pt">
<div>
<p class="MsoNormal">Would making Timeline to use WeakReferences solve the issue \
without the need for a new API?<o:p></o:p></p> <p \
class="MsoNormal"> <o:p></o:p></p> <p class="MsoNormal">-andy<o:p></o:p></p>
<p class="MsoNormal"> <o:p></o:p></p>
<p class="MsoNormal"> <o:p></o:p></p>
<p class="MsoNormal"> <o:p></o:p></p>
<div id="mail-editor-reference-message-container">
<div>
<div style="border:none;border-top:solid #B5C4DF 1.0pt;padding:3.0pt 0in 0in 0in">
<p class="MsoNormal" style="margin-bottom:12.0pt"><b><span \
style="font-size:12.0pt;font-family:"Aptos",sans-serif;color:black">From: \
</span></b><span style="font-size:12.0pt;font-family:"Aptos",sans-serif;color:black">openjfx-dev
<a href="mailto:openjfx-dev-retn@openjdk.org"><openjfx-dev-retn@openjdk.org></a> \
on behalf of John Hendrikx <a \
href="mailto:john.hendrikx@gmail.com"><john.hendrikx@gmail.com></a><br> \
<b>Date: </b>Friday, December 15, 2023 at 21:53<br> <b>To: </b>openjfx-dev <a \
href="mailto:openjfx-dev@openjdk.org"><openjfx-dev@openjdk.org></a><br> \
<b>Subject: </b>New API: Animation/Timeline improvement</span><o:p></o:p></p> </div>
<div>
<p class="MsoNormal" style="margin-bottom:12.0pt"><span style="font-size:11.0pt">Hi \
list,<br> <br>
I've noticed that Animations and Timelines are often a source of leaks, <br>
and their clean-up is often either non-existent or incorrect. The <br>
reason they cause leaks easily is because a running animation or <br>
timeline is globally referred from a singleton PrimaryTimer. The <br>
animation or timeline then refers to properties or event handlers which <br>
refer to controls (which refer to parents and the entire scene).<br>
<br>
For example:<br>
<br>
- ScrollBarBehavior uses a Timeline, but neglects to clean it up. If it <br>
was running at the time a Scene is detached from a Window, and that <br>
Scene is left to go out of scope, it won't because Timeline refers it; <br>
this can happen if the behavior never receives a key released event.<br>
<br>
- ScrollBarBehavior has no dispose method overridden, so swapping Skins <br>
while the animation is running will leave a Timeline active (it uses <br>
Animation.INDEFINITE)<br>
<br>
- SpinnerBehavior has flawed clean up; it attaches a Scene listener and <br>
disables its timeline when the scene changed, but the scene doesn't have <br>
to change for it to go out of scope as a whole... Result is that if you <br>
have a spinner timeline running, and you close the entire window (no <br>
Scene change happens), the entire Scene will still be referred. It also <br>
uses an indefinite cycle count. It also lacks a dispose method, so <br>
swapping Skins at a bad moment can also leave a reference.<br>
<br>
I think these mistakes are common, and far too easy to make. The most <br>
common use cases for animations revolve around modifying properties on <br>
visible controls, and although animations can be used for purposes other <br>
than animating UI controls, this is extremely rare. So it is safe to <br>
say that in 99% of cases you want the animation to stop once a some Node <br>
is no longer showing. For both the mentioned buggy behaviors above, this <br>
would be perfect. A spinner stops spinning when no longer showing, and <br>
a scroll bar stops scrolling when no longer showing. It is also likely <br>
to apply for many other uses of timelines and animations.<br>
<br>
I therefore want to propose a new API, either on Node or Animation (or <br>
both):<br>
<br>
/**<br>
* Creates a new timeline which is stopped \
automatically when this Node<br> * is no longer \
showing. Stopping timelines is essential as they <br> may refer<br>
* nodes even after they are no longer used anywhere, \
preventing <br> them from<br>
* being garbage collected.<br>
*/<br>
Node.createTimeline(); // and variants with the \
various Timeline <br> constructors<br>
<br>
And/or:<br>
<br>
/**<br>
* Links this Animation to the given Node, and stops \
the animation<br> * automatically when the Node is no \
longer showing. Stopping <br> animations<br>
* is essential as they may refer nodes even after they \
are no <br> longer used<br>
* anywhere, preventing them from being garbage \
collected.<br> */<br>
void stopWhenHidden(Node node);<br>
<br>
The above API for Animation could also be provided through another <br>
constructor, which takes a Node which will it be linked to.<br>
<br>
Alternatives:<br>
<br>
- Be a lot more diligent about cleaning up animations and timelines <br>
(essentially maintain the status quo which has led to above bugs)<br>
- Use this lengthy code fragment below:<br>
<br>
Timeline timeline = new Timeline();<br>
<br>
someNode.sceneProperty()<br>
\
.when(timeline.statusProperty().map(status -> status != <br> Status.STOPPED))<br>
.flatMap(Scene::windowProperty)<br>
\
.flatMap(Window::showingProperty)<br> \
.orElse(false)<br> \
.subscribe(showing -> {<br> \
if \
(!showing) timeline.stop();<br> \
});<br> <br>
The `when` line ensures that the opposite problem (Nodes forever <br>
referencing Timelines) doesn't occur if you are creating a new Timeline <br>
for each use (not recommended, but nonetheless a common occurrence).<br>
<br>
--John</span><o:p></o:p></p>
</div>
</div>
</div>
</div>
</blockquote>
</div>
</div>
</div>
</body>
</html>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic