[CH4ACKO3]

Posting a notification can block the render thread on the worker monitor, and a notification between checking the queue and waiting can be lost. Register workers and use park/unpark permits to avoid both races.

[lap2ka]

Holy slop

[CH4ACKO3]

Holy slop

I understand general asynchronous programming concepts, though Java isn't part of my usual tech stack. Essentially, my workflow involves identifying an issue, having the AI ​​pinpoint and explain it in a language-agnostic way, and then proposing a fix for the AI ​​to implement.

Specifically, the issue here is a race condition regarding the queue state: consumers (worker threads) aim to process continuously until the queue is drained before waiting, while producers attempt to send notifications after submitting tasks. A gap exists where a notification sent between the moment a consumer determines the queue is drained and the moment it actually begins waiting gets lost, because the receiver isn't yet ready to wait. A viable fix is ​​to switch to the park/unpark methods provided by LockSupport. Since unpark retains a permit—effectively caching the fact that a notification occurred—a subsequent call to park returns immediately; this seems to me sufficient to resolve the problem.

It was an oversight on my part not to review the final PR before the automatic submission; the AI ​​introduced unnecessary tests while implementing the fix, creating extra overhead for the review process, and I apologize for that. The "Slop" part is decoupled and can be safely removed. However, given the constraints on human time and energy, the best I could do was offer a constructive proposal based on my level of expertise.

Thank you for the feedback, regardless.

[IThundxr]

Is there a related/relevant issue this fixes? I'm a bit weary of merging something like this and changing existing behavior if there isn't documented issue.

[CH4ACKO3]

Is there a related/relevant issue this fixes? I'm a bit weary of merging something like this and changing existing behavior if there isn't documented issue.

I don't have a separate issue linked to this PR. Several players on my server reported intermittent frame drops, mostly on lower-end PCs. I investigated by restricting my test client to four logical processors and found that the render thread could stall while acquiring Flywheel's worker notification lock. These stalls were much less noticeable without the CPU restriction.

One JFR event from the test (Minecraft 1.20.1, Flywheel 1.0.5, Windows, Java 21):

Thread:         Render thread
Event:          jdk.JavaMonitorEnter
Duration:       2336.2775 ms
Previous owner: Flywheel Task Executor #2

ThreadGroupNotifier.postNotification()
ParallelTaskExecutor.execute()
ForEachPlan.execute()
...

The same recording contains 242 ms and 456 ms stalls at this location. That's what prompted this fix: notifying workers shouldn't leave the render thread waiting on their monitor for seconds. The PR uses park/unpark to avoid that contention and also covers the check-to-wait notification gap found during code review.

[CH4ACKO3]

On sufficiently powerful machines, this may have no noticeable impact, which can make it difficult to track down. But it does affect players who are still using lower-end hardware (and hardware prices over the past couple of years may have made upgrading harder for them.)