On Tue, 14 Oct 2025 08:53:11 GMT, Johan Vos <[email protected]> wrote:
>> I know you do - the links were added for sake of others :-) >> >> The answer to your question is - I don't know. If one can guarantee that >> there is only one thread accessing it, we may need to add a comment (though >> things might change in the future, and no one reads comments elsewhere in >> the code). And if not, then some form of synchronization is mandatory. >> >> I prefer not to think though, and make sure we don't plant booby traps in >> the code. >> >> BTW, what is the cost of volatile? 0.1 ns? ;-) > >> I know you do - the links were added for sake of others :-) > > very kind ;) > >> The answer to your question is - I don't know. If one can guarantee that >> there is only one thread accessing it, we may need to add a comment (though >> things might change in the future, and no one reads comments elsewhere in >> the code). And if not, then some form of synchronization is mandatory. > > I ran all system tests to see when/how the active field was read/modified and > on which thread. All operations did happen on the main/eventThread, hence > single-threaded. Since all of this is about processing events for the > eventThread, this makes sense. But to make sure, I did the following research: > > There are 3 places in NestedRunnableProcessor where the active field is used > (read or write) > > `Object newRunLoop()` > > The first time this is invoked is when the `HeadlessApplication` starts, when > `Application.run` is called. This creates a new Thread, which is the > eventThread. The first thing this thread does is invoking `runLoop` (hence, > it is executed on the eventThread). > Apart from the first time, this is invoked from > `Application.enterNestedEventLoop`. which clearly states in its javadoc that > the method may only be invoked on the main (event handling) thread, and it > even does a `checkEventThread()` for this. > > `void leaveCurrentLoop()` > > This is invoked from `Application.leaveNestedEventLoop`, which has the same > docs + check as `enterNestedEventLoop` > > `void stopProcessing()` > > This is invoked from Application.terminate, which starts with a > `checkEventThread` > > In summary, all operations in the NestedRunnableProcessor or controlled/gated > by the com.sun.glass.ui.Application which makes sure that all access is > guarded by `checkEventThread` > >> I prefer not to think though, and make sure we don't plant booby traps in >> the code. > I don't think I follow this approach. There are many concerns about "JavaFX > being slow", so if we can avoid it, I think we should. That doesn't give > green light to plant booby traps of course ;) >> > >> BTW, what is the cost of volatile? 0.1 ns? ;-) > > I'm not an expert, but I believe it is more than 1 ns on "typical desktop > systems". Since this is evaluated on every invocation of a Runnable on the > AppThread (hence all pulses), plus on every Runnable executed via e.g. > Platform.runLater(), this is something that can be invoked very often. > > If there was even a single cornercase that we could not rule out, or document > as "you're on your own here", I would absolutely agree to make it ... Thank you for the analysis, very helpful! I would like to ask for a comment to be added, explaining the design. The main reason is I've seen too many times when the code was broken because the original assumptions were no longer valid, long after the knowledgeable people have left. At least the comment might help future reviewers. (I'll re-approve if you decide to add the comment). BTW, you are right - according to https://shipilev.net/blog/2014/on-the-fence-with-dependencies/ the overhead of `volatile` might be a few nanoseconds on modern systems. ------------- PR Review Comment: https://git.openjdk.org/jfx/pull/1934#discussion_r2429611866
