On Tue, 14 Oct 2025 15:30:20 GMT, Andy Goryachev <[email protected]> wrote:
>>> 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 ... > > 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. Glass is single-threaded by design, except for those methods that may be called on any thread. So I don't think a detailed comment is needed. A brief comment saying "this is only accessed on the JavaFX application thread (or event thread) might be helpful. ------------- PR Review Comment: https://git.openjdk.org/jfx/pull/1934#discussion_r2429669667
