[External] : Re: HEADS-UP: Threading restriction for Animation play, pause, stop now enforced USE CASE
Kevin Rushforth
kevin.rushforth at oracle.com
Wed Jan 24 18:42:19 UTC 2024
I also now favor option 2 and was in the process of writing something up
recommending that we do that. Phil and I (and a couple others) had an
offline discussion and think this is the way to go.
Given the short amount of time to get this into 22, I will file the JBS
issue in the next hour or so.
Nir: if you want to take it, that would be great. Otherwise, I will do
it. We need the PR and CSR filed before the end of this week.
Regarding other methods that choose option 1, many of them are more
complicated (e.g., scene mutation can be done on a background thread as
long as the scene is not "showing") or must be synchronous. Animation
starts something that conceptually happens later on the FX animation
thread anyway, so wrapping it in a runLater (if not already on the right
thread) doesn't really change the semantics in an appreciable way.
-- Kevin
On 1/24/2024 10:26 AM, Nir Lisker wrote:
> These are the options I see as reasonable:
>
> 1. Document that these methods *must* be run on the FX thread and
> throw an exception otherwise. This leaves it to the responsibility of
> the user. However, this will cause the backwards compatibility
> problems that Jugen brought up. As a side note, we do this in other
> methods already, but I always questioned why let the developer do
> something illegal - if there's only one execution path, force it.
> 2. Document that these methods *are* run on the FX thread (the user
> doesn't need to do anything) and change the implementation to call
> runLater(...) internally unless they are already on the FX thread.
> This will be backwards compatible for the most part (see option 3).
> The downside might be some surprise when these methods behave differently.
> 3. Document that it's *recommended* that these methods be run on the
> FX thread and let the user be responsible for the threading. We can
> explain that manipulating nodes that are attached to an active
> scenegraph should be avoided.
>
> I prefer option 2 over 1 regardless of the backwards compatibility
> issue even, but would like to know if I'm missing something here
> because in theory this change could be done to any "must run on the FX
> thread" method and I question why the user had the option to get an
> exception.
> Option 3 is risky and I wager a guess that it will be used wrongly
> more often than not. It does allow some (what I would call) valid
> niche uses. I never did it.
>
> On Wed, Jan 24, 2024 at 4:21 PM Kevin Rushforth
> <kevin.rushforth at oracle.com> wrote:
>
> I'd like to hear from the others on this. I don't see any
> fundamental problem with having the play/pause/stop methods wrap
> their implementation in a runLater (if not on the FX Application
> thread already), and documenting that it does so, if we can get
> general agreement.
>
> -- Kevin
>
>
> On 1/24/2024 5:29 AM, Jurgen Doll wrote:
>> Hi Kevin
>>
>> If I may make one more final appeal then to an alternative
>> solution please.
>>
>> Could we then instead of throwing an Exception rather invoke
>> runLater if needed inside play, stop, and resume.
>>
>> Putting the onus on the developer is fine if it is the developer
>> that is invoking the call, but if it's in a library then it's a
>> no go.
>>
>> In my application I have two libraries that I know of where this
>> happens. The major problem is that with FX22 as it now stands my
>> application just crashes because play() does an FX thread check
>> and throws an Exception which it never did before. There are
>> bound to be other applications out there that are going to find
>> themselves in a similar position.
>>
>> PLEASE !
>>
>> Regards
>> Jurgen
>>
>>
>>
>> On Wed, 24 Jan 2024 15:15:31 +0200, Kevin Rushforth
>> <kevin.rushforth at oracle.com> <mailto:kevin.rushforth at oracle.com>
>> wrote:
>>
>> Thank you to Jurgen for raising the question and to Nir,
>> John, and Michael for evaluating it.
>>
>> I conclude that there is insufficient motivation to revert
>> the change in behavior implemented by JDK-8159048 to allow
>> calling the play/pause/stop methods of Animation on a
>> background thread. Doing so without making it fully
>> multi-thread-safe would be asking for problems, and making it
>> fully multi-thread-safe would be a fair bit of work to do it
>> right without a clear benefit.
>>
>> We will proceed with the current approach and let JDK-8159048
>> stand. Further, we will proceed with
>> https://bugs.openjdk.org/browse/JDK-8324219 which is under
>> review in https://github.com/openjdk/jfx/pull/1342
>> <https://urldefense.com/v3/__https://github.com/openjdk/jfx/pull/1342__;!!ACWV5N9M2RV99hQ!PLtaZAvaobOk-fe_DRSy2AHiPOFUaK683TVXWIvmjnTiUGhmBp3x_GIy_wlMuEV3IHTUMx37HxJHiVuWiFRR$>
>>
>> -- Kevin
>>
>> On 1/24/2024 12:30 AM, Nir Lisker wrote:
>>> After playing around with the code sample, I think that this
>>> is not the right way to use the animation. The reason is
>>> that there is no point in starting the animation before the
>>> control is attached to the scenegraph, or even made visible.
>>> A small refactoring where, e.g., the controller class
>>> exposes a method to start the animation in onSucceeded or
>>> just calls it on the FX thread is enough. I never start an
>>> animation as part of the construction because it's not the
>>> right time. John suggested tying the lifecycle of the
>>> animation to the showing of the node, which also solves the
>>> problem.
>>>
>>> There are animations like PauseTransition or other
>>> non-interfering Timelines that could reasonably be run on a
>>> background thread. Or maybe just on an unconnected control.
>>> This could be a reason to not limit animation methods to the
>>> FX thread at the expense of possible user errors, but
>>> document the pitfall.
>>>
>>> I don't see a good use case for modifying controls in a
>>> background thread while still interacting with the
>>> scenegraph, hence for adding multithread support.
>>>
>>> - Nir
>>>
>>> On Mon, Jan 22, 2024, 12:59 Jurgen Doll
>>> <javafx at ivoryemr.co.za> wrote:
>>>
>>> Here's an example as requested by Nir:
>>>
>>> publicclassFxTimeLineTest extendsApplication
>>>
>>> {
>>>
>>> privateBorderPane bp= newBorderPane( newLabel("Loading") );
>>>
>>> publicstaticvoidmain( String[] args) {
>>>
>>> launch( FxTimeLineTest.class, args);
>>>
>>> }
>>>
>>> @Override
>>>
>>> publicvoidstart( Stage primaryStage) throwsException {
>>>
>>> newThread( newLoadScene() ).start();
>>>
>>> primaryStage.setScene( newScene( bp, 300, 200 ) );
>>>
>>> primaryStage.setTitle( "Memory Usage");
>>>
>>> primaryStage.show();
>>>
>>> }
>>>
>>> privateclassLoadScene extendsTask<Parent> {
>>>
>>> @OverrideprotectedParent call() throwsException {
>>>
>>> Parent p= FXMLLoader.load( getClass(
>>> ).getResource("TestView.fxml") );
>>>
>>> Thread.sleep( 1000 );
>>>
>>> returnp;
>>>
>>> }
>>>
>>> @Overrideprotectedvoidsucceeded() {
>>>
>>> bp.setCenter( getValue() );
>>>
>>> }
>>>
>>> @Overrideprotectedvoidfailed() {
>>>
>>> getException().printStackTrace();
>>>
>>> }
>>>
>>> }
>>>
>>> }
>>>
>>> ------------------------------------------------------------------------------------------------------
>>>
>>>
>>> publicclassTestView
>>>
>>> {
>>>
>>> @FXMLprivateLabel memory;
>>>
>>> privatestaticfinaldoubleMEGABYTE= 1024 * 1024;
>>>
>>> @FXMLprivatevoidinitialize()
>>>
>>> {
>>>
>>> varupdater= newTimeline
>>>
>>> (
>>>
>>> newK eyFrame( Duration.seconds(2.5), event->
>>>
>>> {
>>>
>>> varruntime= Runtime.getRuntime();
>>>
>>> doublemaxMemory= runtime.maxMemory() / MEGABYTE;
>>>
>>> doubleusedMemory= (runtime.totalMemory() -
>>> runtime.freeMemory()) / MEGABYTE;
>>>
>>> memory.setText( (int) usedMemory+ " MB / "+ (int)
>>> maxMemory+" MB");
>>>
>>> })
>>>
>>> );
>>>
>>> updater.setCycleCount(Animation.INDEFINITE); // This
>>> FXML is being loaded on a background thread
>>>
>>> updater.play();
>>>
>>> }
>>>
>>> }
>>>
>>> ------------------------------------------------------------------------------------------------------
>>>
>>>
>>> TestView.fxml
>>>
>>> <?xml version="1.0" encoding="UTF-8"?>
>>>
>>> <?import javafx.scene.control.Label?>
>>>
>>> <?import javafx.scene.layout.StackPane?>
>>>
>>> <StackPane xmlns:fx="http://javafx.com/fxml/1
>>> <https://urldefense.com/v3/__http://javafx.com/fxml/1__;!!ACWV5N9M2RV99hQ!PLtaZAvaobOk-fe_DRSy2AHiPOFUaK683TVXWIvmjnTiUGhmBp3x_GIy_wlMuEV3IHTUMx37HxJHia1cBAoi$>"
>>> fx:controller="TestView">
>>>
>>> <children>
>>>
>>> <Label fx:id="memory" text="Current / Max MB" >
>>>
>>> <properties hashCode="12345" />
>>>
>>> </Label>
>>>
>>> </children>
>>>
>>> </StackPane>
>>>
>>>
>>>
>>>
>>> On Sat, 20 Jan 2024 17:08:41 +0200, Nir Lisker
>>> <nlisker at gmail.com> wrote:
>>>
>>> Hi Jurgen,
>>>
>>> What I'm confused about the most is what it is you
>>> are actually trying to do that necessitates the use
>>> of animations outside of the FX thread. You said
>>> that you need to initialize controls on another
>>> thread, and that you are using Task (both of which
>>> are fine), but how does playing animations relate?
>>> Playing an animation is something that is done
>>> explicitly, usually in order to manipulate data. Can
>>> you give a real use case, like a minimized version
>>> of what you're doing?
>>>
>>> - Nir
>>>
>>
>>
>>
>>
>> --
>> Using Opera's mail client: http://www.opera.com/mail/
>> <https://urldefense.com/v3/__http://www.opera.com/mail/__;!!ACWV5N9M2RV99hQ!PLtaZAvaobOk-fe_DRSy2AHiPOFUaK683TVXWIvmjnTiUGhmBp3x_GIy_wlMuEV3IHTUMx37HxJHifb0G69Z$>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/openjfx-dev/attachments/20240124/3bd467b3/attachment-0001.htm>
More information about the openjfx-dev
mailing list