JDK-8189926 : [Mac] Pulse timer should pause when idle
  • Type: Bug
  • Component: javafx
  • Sub-Component: other
  • Affected Version: 8,9,10
  • Priority: P3
  • Status: Resolved
  • Resolution: Fixed
  • OS: os_x
  • Submitted: 2017-10-25
  • Updated: 2020-02-04
  • Resolved: 2018-12-24
The Version table provides details related to the release that this issue/RFE will be addressed.

Unresolved : Release in which this issue/RFE will be addressed.
Resolved: Release in which this issue/RFE has been resolved.
Fixed : Release in which this issue/RFE has been fixed. The release containing this fix may be available for download as an Early Access Release or a General Availability Release.

To download the current JDK release, click here.
JDK 8 Other
8u212Fixed openjfx12Fixed
Related Reports
Relates :  
JDK-8214040 - Optimize pulse generation on windows
Relates :  
JDK-8164438 - JavaFX changes Windows interrupt frequency to 1ms
Relates :  
JDK-8238505 - [macOS] Intermittent deadlock on exit in MacTimer
Relates :  
JDK-8214041 - Optimize pulse generation on Linux
Description
By design, JavaFX should generate ~60 pulses per second on all platforms.
On Mac platform, number of pulses vary from approximately 30 to more than 250.
These additional number of pulses result in more power consumption and drains considerable amount battery.

Further, JavaFX should not generate pulses when there are no animations or changes to the scene graph.
Comments
[~arapte] please back port to 8u
24-12-2018
Changeset: 4e1e2f56c7af Author: arapte Date: 2018-12-24 13:23 +0530 URL: http://hg.openjdk.java.net/openjfx/jfx-dev/rt/rev/4e1e2f56c7af
24-12-2018
looks fine ... +1
21-12-2018
The .08 webrev looks fine. Regarding the animation NPEs, they should go away if you change your test program to call play and stop on the JavaFX Application Thread, like this: Platform.runLater(() -> rotate.play()); As mentioned in JDK-8159048 the animation methods are not thread-safe. I note that your test program stops and starts the animation every 200 msec, which is faster than the 250 msec threshold for pausing the timer. It���s now a good test of the pause threshold, but is no longer a good test for any potential leaks, so you might want to run the test with a duration of 400. One minor style nit: > private final static long PAUSE_THRESHOLD_DURATION = 250; The usual order is "private static final" (static comes before final). +1 (no need for a new webrev to swap the order of final and static)
21-12-2018
Hello Kevin, 1. The sample program starts and stops animation on a temporary thread. which is similar to the test program in JDK-8159048. Reverted the change as suggested. 2, 3 Changed as per comment. Please take a look at the updated webrev: http://cr.openjdk.java.net/~arapte/fx/8189926/webrev.08/ Verified all test run on Mac & Windows. And Ensemble app on all three platforms. Did not observe any failures. Ubuntu takes more time for test run, shall run the tests on Ubuntu later in evening.
20-12-2018
Maybe related to the animation issue, see JDK-8159048
18-12-2018
1. The changes synchronizing the animation are unrelated to this fix and should be reverted. It's possibly a bug in your test program (e.g., if you are making calls to a running animation from a thread other than the JavaFX Application Thread), but even if a fix is needed, it should be done separately. 2. Minor: resumeTimer can reset pauseRequested unconditionally (and no need to set firstPauseRequestTime at all). 3. Minor: PAUSE_THRESHOLD_DURATION can be static The rest looks fine, but I haven't tested it yet.
18-12-2018
Hello Kevin, Please review this updated webrev: http://cr.openjdk.java.net/~arapte/fx/8189926/webrev.07 Changes: 1. Added pauseTimer() and resumeTimer() methods in QuantumToolkit. Timer thread shall paused after 250ms from first pause request. 2. While testing the sample observed that It arbitrarily throws NPE at line #562 of changed code. NPE can be observed once or twice in 5 to 10 minutes of execution. The behavior looks like, animationRunnable gets set to null from the temp thread of sample when animation is stopped, and at the same time animationRunnable gets accessed at line #562. Hence as a fix the two code blocks where animationRunnable is accessed are synchronized using lockAnimationRunnable. Lines: #559, #566, #887, #889, and #894 are the changes to fix the NPE, removing only these changes shall cause the NPE with the sample program. Regarding the sanity testing, I did verify the webrev.05 on Win and Ubuntu. But have not verified the updated webrev yet.
14-12-2018
I'll take a quick look at your sample, but the numbers seem fine (I assume you mean with/with the "fix" rather than with/with "Fx"). Regarding your last comment about the oscillation: > So may be we can use time instead of pulse drop count? something like, If pulse are dropped for 100 to 250 ms, then pause the timer. Yes, that sounds like a better idea. And given your findings above I would probably lean towards 250 (or even 500) msec as an idle timeout. That would also smooth out the pause/resume from mouse movement.
13-12-2018
Regarding the memory leak test. The change does not seem to cause any memory leak. test used for verifying: http://cr.openjdk.java.net/~arapte/fx/8189926/RotateTransitionApp.java The program plays a rotation animation for 200 ms and then stops for 200 ms. This play and stop runs in loop. Verified the sample with -Xmx50m & -Xmx5m. The Runtime.getRuntime().freeMemory() never reduces below certain amount. This is because gc claims the memory whenever necessary. The sample prints minimum and maximum amount of free memory during the execution. The variation in free memory and gc of memory, looks same with and without fix. Free memory values when sample is executed for approximately 10 mins with -Xmx5m -----------------With Fx----------------- > totalMemory() : 6291456 > freeMemory() : 2772616 > MIN free memory : 1571880 > MAX free memory : 3141976 -----------------Without Fx----------------- > totalMemory() : 6291456 > freeMemory() : 2249752 > MIN free memory : 1985800 > MAX free memory : 3031792 -------------------------------------------------
13-12-2018
Hi Kevin, This oscillation between pause/resume looks like, video may have only 60 or lesser frames per second, and more number(300-400) of pulses generated. So we don't really have anything to update on each pulse. I was also thinking of using some minimum count of dropped pulses before pausing. I tried it with 5 & 20, the pause/resume oscillation reduces noticeably with 20 and comparatively observed more oscillation with 5. Number of pulses per second are different platform wise and implementation wise. like, CVDisplayLink thread generates (100 to 300+) pulses but "Glass Timer Thread" would generate 60. So may be we can use time instead of pulse drop count? something like, If pulse are dropped for 100 to 250 ms, then pause the timer.
13-12-2018
One thought I had is that the issue of oscillating between pause/resume would be solved for the common cases if we waited for 2 or 3 additional pulses after the request to pause before actually calling Timer::pause. This could be accomplished by having "pause" and "resume" methods in QuantumToolkit, which would be called everywhere you currently call pulseTimer::pause/resume. They could do something like this: synchronized pause() { if (pausePending) { if (--pulseCount == 0) { pausePending = false; pulseTimer.pause(); } } else { pausePending = true; pauseCount = TIMER_PAUSE_PULSE_COUNT; // 2 is probably sufficient } } synchronized resume() { pausePending = false; pulseTimer.resume(); }
13-12-2018
One other question: if you haven't already done so, can you do a quick build / sanity test on Windows and Linux?
13-12-2018
As a follow-up, when I run one of the other Ensemble8 media tests (Streaming Media Player) it does a pause / resume frequently enough that it might cause performance issues. The media still played fine, but I wonder if there are other cases where we will frequently oscillate between the pause and resume states.
13-12-2018
The new version looks good. I do note that in the case of using a Java Timer Thread, that thread will wake up every 16 msec even while paused, although it won't call the Java runnable. Since this is just a fallback case on Mac it isn't worth the effort of designing something more complex to avoid that. I did see one interesting thing (not necessarily a problem) when running Ensemble8 media samples or HelloMedia. I instrumented the code and can see it periodically pause and then resume the thread. I would have expected it to not pause while the media was playing, but I suspect that it really is going idle for some reason. It might be something to file a follow-up issue to investigate. Let me know what the results of the long-running test are. I'm about ready to approve this pending the results of that test.
13-12-2018
Hi Murali & Kevin, Thanks for the review, Please take a look at the updated webrev: http://cr.openjdk.java.net/~arapte/fx/8189926/webrev.06/ Changes as per the review comments, [~mbilla] 1, 5, 6, 7, 8 & 10 Changed as per the comment. 2. Removing variable name 'cls' throws compiler error: parameter name omitted 3. nullptr throws error: use of undeclared identifier 'nullptr' 4. timer is an instance of GlassTimer. It is initialized only once and should never be null. It would take something like memory corruption for it to be NULL, so the NULL check can be skipped. 9. _1pause() will be called on the timer (CVDisplayLink) thread itself, so the if condition should pass every time and CVDisplayLinkStop() should be executed. But anyway adding the log in else to catch any error case. The similar log can not be added to _1resume(). _1resume() gets called multiple times and in cases when the thread is already resumed the log would get printed multiple times. [~kcr] > Long-running test I have not tested this, I shall test and update. > comment in Timer.java Updated the comment separately for pause and resume. > PaintCollector.java Changed setDirty() method to private. > Pause/Resume changes for "Glass Timer Thread" 1. Added a BOOL _paused; member to GlassTimer.h :: GlassTimer 2. This member shall control the call to GlassTimer->_runnable inside while loop of "Glass Timer Thread" 3. Removed the variable detachCVDisplayLinkThread from previous webrev, and using GlassTimer->_paused instead. 4. Tested the change with "-Djavafx.animation.fullspeed=true" & by setting nativeSystemSync to true and false[QuantumToolkit.java::Line#372]
12-12-2018
General: Have you done any long-running testing to make sure that there are no memory leaks as a result of detaching / reattaching to the CVDisplayLink Thread? A test that animates for a time, then pauses for a time (making sure that no FX animations are running). Timer.java: I think you can remove the comment about "Currently implemented only for mac platform." Since it is an abstract method it is up to each implementation to decide what to do with it (the comment isn't needed in the base class). PaintCollector.java: setDirty can be private. GlassTimer.m: The changes looks OK for the "timer->_period == 0" case (the case where we are using the CVDisplayLink), but not for the case of _period > 0, when we are using the Java timer thread. In that mode, it will never resume, once paused. This breaks "-Djavafx.animation.fullspeed=true" and would also break if MacApplication::staticScreen_getVideoRefreshPeriod ever returned 0.
10-12-2018
[~arapte] I will run on mac and will update the results: Few comments from code review: 1. In QuantumToolkit.java, as per below new changes, i guess "QT.postPulse DROP :" will be printed whenever "Pause timer" occurs in debug mode. Should the code be like below? } else if (!animationRunning.get() && !nextPulseRequested.get() && !pulseRunning.get()) { pulseTimer.pause(); if (debug) { System.err.println("QT.postPulse#(" + System.nanoTime() + "): Pause Timer : " + pulseString()); } } else if (debug) { System.err.println("QT.postPulse#(" + System.nanoTime() + "): DROP : " + pulseString()); } 2. In GlassTimer.m, since "cls" is un-used, you can just remove the variable name "cls" from Java_com_sun_glass_ui_mac_MacTimer__1pause and Java_com_sun_glass_ui_mac_MacTimer__1resume. 3. Need to correct the method signature in description comments for below 2 method in GlassTimer.m. Java_com_sun_glass_ui_mac_MacTimer__1pause and Java_com_sun_glass_ui_mac_MacTimer__1resume Signature: ()I -> should be changed to Signature: ()V 4. In GlassTimer.m, you are not checking timer for NULL. is it not required ? GlassTimer *timer = (GlassTimer*)jlong_to_ptr(jTimerPtr); 283 if (timer->_period == 0) 284 { 285 // stop the display link 5. In GlassTimer.m, please remove extra space in between "*" and "env" for Java_com_sun_glass_ui_mac_MacTimer__1pause and Java_com_sun_glass_ui_mac_MacTimer__1resume (JNIEnv * env, jclass cls, jlong jTimerPtr) change to (JNIEnv *env, jclass cls, jlong jTimerPtr) 6. In GlassTimer.m, I think variable name of return value for CVDisplayLinkStart will be appropriate from "err" to "ret". CVReturn err = CVDisplayLinkStart(GlassDisplayLink); to CVReturn ret = CVDisplayLinkStart(GlassDisplayLink); if (ret != kCVReturnSuccess) 7. In GlassTimer.m, you need to check return value of CVDisplayLinkStop() in Java_com_sun_glass_ui_mac_MacTimer__1pause and try to print a log ? As per https://developer.apple.com/documentation/corevideo/1457281-cvdisplaylinkstop, "If the specified display link is already stopped, CVDisplayLinkStop returns an error. 8. In GlassTimer.m, already there is a log message for "NSLog(@"CVDisplayLinkStart error: %d", err);" in the method Java_com_sun_glass_ui_mac_MacTimer__1initIDs. Now we have the same log in Java_com_sun_glass_ui_mac_MacTimer__1resume also. I feel better to add the method name for the log while printing in Java_com_sun_glass_ui_mac_MacTimer__1resume like NSLog(@"MacTimer__1resume : CVDisplayLinkStart error: %d", err); 9. In GlassTimer.m, regarding the check "if (GlassDisplayLink != NULL && !CVDisplayLinkIsRunning(GlassDisplayLink))" in method Java_com_sun_glass_ui_mac_MacTimer__1resume, in case of GlassDisplayLink is null or displaylink is not running, better to put a log saying resume failed in else condition. if (GlassDisplayLink != NULL && !CVDisplayLinkIsRunning(GlassDisplayLink)) { CVReturn err = CVDisplayLinkStart(GlassDisplayLink); if (err != kCVReturnSuccess) { NSLog(@"CVDisplayLinkStart error: %d", err); } } else { NSLog(@"MacTimer__1resume..CVDisplayLinkStart failed due to GlassDisplayLink is null or GlassDisplayLink not running"); } same thing applies to Java_com_sun_glass_ui_mac_MacTimer__1pause also. 10. You can correct the below statements: a) // The thread is attached as a daemon, it will not stop Java from exiting. To // As the thread is attached as daemon, it will not stop Java from exiting. b) // _pause() calls CVDisplayLinkStop() on current CVDisplayLink thread, which // stops the thread, so the thread should be detached. To // _pause() calls CVDisplayLinkStop() on current CVDisplayLink thread, which // in turn stops the thread. Hence the thread should be detached.
10-12-2018
Please review the fix, Fix: Please do read previous comments for more info. On Mac platform FX uses CVDisplayLink for synchronizing the frames. https://developer.apple.com/documentation/corevideo/cvdisplaylink-k0k Fix is that, JavaFX should not generate pulse when there are no animations or changes to the scene graph. This can be achieved by controlling the CVDisplayLink thread. The thread should be paused when there is nothing to update to frame and resume whenever frame has to be updated. Verification: 1. All test pass with the fix. 2. Testing the fix with Ensemble app. a. Observed no visual artefact with the fix. b. Observed below improvements when application is idle, --> 60 to 80 % reduction in % CPU use --> 72 to 73 % reduction in Idle wake ups --> 75 % reduction in Energy impact Thanks Kevin for the reviews and guidance, Some of the important comments received and addressed are, 1. Verify which threads might pause and resume the timer and verify if any race conditions. 2. Call QuantumToolkit.requestNextPulse() from PaintCollector instead of checking if PaintCollector has dirty scene from QuantumToolkit. 3. CVDisplayLink thread cannot be paused, so it must be attached and detached with resume & pause respectively. => suggested alternatives to avoid attaching-detaching, a. Attach the thread only once. b. Setting a NULL callback to CVDisplayLink => Both the alternatives do not work. 4. Add synchronized abstract pause and resume methods to Timer and update respective platform specific timer class implementations. please review the webrev.05 updated as per the above comments: http://cr.openjdk.java.net/~arapte/fx/8189926/webrev.05/
28-11-2018
Given that the proposed fix automatically pauses and resumes the timer without any application changes, this is really a bug fix rather than an RFE. It will not need a CSR as there are no interface changes. It's a fix for a performance bug.
09-10-2018
The timer cannot be paused in 9 scenarios as listed in previous comment. All of the scenario's set an AnimationRunnable which in turns sets animationRunning to true, and so the timer cannot be paused. Here is reason for each scenario. 1. WebView instance Reason: WebEngine uses AnimationTimer to guarantee constant pulse activity. (See: RT-14433) 2. Timeline animation of INDEFINITE cycle & the animation is not stopped or paused. Reason: This is expected behavior with INDEFINITE cycle animation. The application should take care of stopping or pausing the INDEFINTE cycle animation. 3. ColorPicker & when mouse is hovered on color palette. Reason: ColorPalette Installs tooltip (Line 515). Tooltip uses Timeline animation to make sure that the tooltip is marked as activated. 4. Text control & when cursor is blinking: Reason: TextInputControl uses a Timeline animation to show blinking of cursor. (TextInputControlSkin::CaretBlinking::Timeline caretTimeline). 5. Text control & text is selected. Reason: In continuation to point 4, the caret blinking animation remains in progress while text is being selected. 6. Spinner control & when a spinner control get focus. Reason: Spinner uses FakeFocusTextField <extends TextField> as editor. The TextField's caret blinking Timeline animation gets played. 7. Ensemble::3D Sphere sample Reason: The earth���s RotateTransition animation is played with INDEFINITE cycle count. 8. Ensemble::appLogin -> Enter user name - Password -> Page navigates to next page but timer does not pause. Reason: TextInputControlSkin::CaretBlinking::Timeline caretTimeline animation remains in progress. 9. Sample MandelbrotSet Reason: This sample explicitly sets an AnimationRunnable, i.e. a DelayedRunnable through MasterTimer (implements AbstractMasterTimer).
09-10-2018
Progress: Functionality: 1. Added pause / resume APIs to pause and resume the pulse timer which reduces wake ups of application. 2. The pulse timer shall be paused when there is no pending update on JavaFX side. i.e. when the pulse is being dropped (i.e. When there is no animation in progress & next pulse is not requested & scene graph has no dirty node & previous pulse is not in progress) Checking that scene graph has no dirty node is an additional check, whenever a node is set to be dirty next pulse is requested. So next pulse requested check should be sufficient. 3. Pulse timer will be resumed when animation is in progress OR next pulse is requested. Verification: 4. Testing the fix with Ensemble app. 4.1 Observed no visual artefact with the fix. 4.2 Observed below improvements when application is idle, --> 60 to 80 % reduction in % CPU use --> 72 to 73 % reduction in Idle wake ups --> 75 % reduction in Energy impact 4.3 More investigation is required for below scenarios where the pulse timer does not get paused when Application has, 1. WebView instance 2. Timeline animation of INDEFINITE cycle & this animation is not stopped or paused. 3. ColorPicker & when mouse is hovered on color palette. 4. Spinner control & when the spinner control is focused. 5. Text control & when cursor is blinking. 6. Text control & the text is selected. And with below samples in Ensemble, 7. 3D Sphere 8. appLogin: enter user name & password -> Press enter -> Page navigates to next -> but pulse timer does not pause 9. Sample MandelbrotSet These scenarios set animation flag to true, which does not allow pulse timer to be paused. These scenarios should not be a hurdle to proceed for review & check in of the fix but should be well investigated and documented here. Shall add more information on each of these scenarios.
09-10-2018
Changing the issue type to Enhancement, since JavaFX works this way by design. We wake up once every pulse (roughly every 16 msec) to process any pending events, tasks, or animation. This causes a tiny drain on the CPU of roughly 0.2% - 0.3%. We could consider an RFE to allow an application to pause JavaFX entirely, or some similar mechanism.
09-10-2018