Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8176499: Dependence on java.util.Timer freezes screen when OS time resets backwards #117

Conversation

@dellgreen
Copy link
Contributor

dellgreen commented Feb 19, 2020

https://bugs.openjdk.java.net/browse/JDK-8176499

This pull request fixes a long standing issue in the MonocleTimer class whereby it has a dependency on the java.uti.Timer class which is dependent on system time and can cause UI freezes for seconds/minutes/hours/days/years dependent upon how far back in time the system clock is set by either a user manually or NTP. This looks like it is because the Timer class will wait for (executionTime - currentTime) before proceeding if a task hasn't fired yet.

https://hg.openjdk.java.net/jdk10/master/file/be620a591379/src/java.base/share/classes/java/util/Timer.java#l553

For a long running embedded device with a UI like IOT devices this is pretty disastrous.
We recently re-discovered this issue whilst testing such a device before going into production.

The MonocleTimer class is used by MonocleApplication and QuantumToolkit class to create its pulseTimer for emebdded systems and sets it up to fire periodically inline with the requested pulse frequency which by default is 60Hz resulting in a pulse interval of 16ms.

It is well documented that for implementations that wish to measure elapsed time ScheduledThreadPoolExecutor should be used as a replacement for java.util.Timer class.

Java Concurrency In Practice:
https://pdfs.semanticscholar.org/3650/4bc31d3b2c5c00e5bfee28ffc5d403cc8edd.pdf (page 77)

"The Timer facility manages the execution of deferred ("run this task in 100 ms") and periodic ("run this task every 10ms") tasks. However, Timer has some drawbacks, and ScheduledThreadPoolExecutor should be thought of as its replacement."

With the original implementation, if I set the date.time back 8 years for example the UI freezes up indefinitely (I cant wait 8 years). Repeating the same test with the proposed implementation has no affect on the UI and it runs as normal.

The proposed solution has been tested on an Arm iMX6 board.

Whist testing in isolation the MonocleTimer class with no work to do on each pulse, it looks like the change from Timer class to ScheduledThreadPoolExecutor also has brought with it a greater accuracy of when the pulses are fired.

The following results were observed when running MonocleTimer at 60Hz for 1 minute. It appears that we get a higher frequency of pulses hitting the 16ms mark with the replacement solution

x86-64 linux desktop:

---- Timer class ----
NumSamples: 3599
Mean: 16.230063906640734
StdDev: 0.45021901536403147
Median: 16
Mode: 16, freq: 2714, perc: 75.40983606557377%

---- Scheduler class ----
NumSamples: 3599
Mean: 16.0
StdDev: 0.0
Median: 16
Mode: 16, freq: 3599, perc: 100.0%

Arm linux iMX6:

---- Timer class ----
NumSamples: 3599
Mean: 16.182272853570435
StdDev: 0.4224950723394174
Median: 16
Mode: 16, freq: 2837, perc: 78.82745207001946%

---- Scheduler class ----
NumSamples: 3599
Mean: 15.995554320644624
StdDev: 0.3666906549602725
Median: 16
Mode: 16, freq: 3468, perc: 96.3601000277855%


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8176499: Dependence on java.util.Timer freezes screen when OS time resets backwards

Reviewers

  • Johan Vos (jvos - Reviewer)

Download

$ git fetch https://git.openjdk.java.net/jfx pull/117/head:pull/117
$ git checkout pull/117

@bridgekeeper
Copy link

bridgekeeper bot commented Feb 19, 2020

👋 Welcome back dellgreen! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request.

@openjdk openjdk bot added the rfr label Feb 19, 2020
@dellgreen
Copy link
Contributor Author

dellgreen commented Feb 19, 2020

@kevinrushforth apologies for previous, my local repos seem to be messed up when i changed remotes from old javafx github repo to new one, as that rogue commit didnt exist my side for some reason. looks like its fixed now

@mlbridge
Copy link

mlbridge bot commented Feb 19, 2020

Webrevs

@kevinrushforth
Copy link
Member

kevinrushforth commented Feb 19, 2020

I don't see any stray commits, so it looks like your branch is based off of master correctly.

One thing I would ask you to change is that the title of this PR should exactly match the title of the JBS bug. So can you change it to:

8176499: Dependence on java.util.Timer freezes screen when OS time resets backwards
@kevinrushforth
Copy link
Member

kevinrushforth commented Feb 19, 2020

Seems a simple enough fix. Probably @johanvos can review it.

@kevinrushforth kevinrushforth requested a review from johanvos Feb 19, 2020
@dellgreen dellgreen changed the title 8176499: Remove MonocleTimer dependency on system time 8176499: Dependence on java.util.Timer freezes screen when OS time resets backwards Feb 19, 2020
@dellgreen
Copy link
Contributor Author

dellgreen commented Feb 19, 2020

I don't see any stray commits, so it looks like your branch is based off of master correctly.

One thing I would ask you to change is that the title of this PR should exactly match the title of the JBS bug. So can you change it to:

8176499: Dependence on java.util.Timer freezes screen when OS time resets backwards

apologies, all done

@littlefreaky
Copy link

littlefreaky commented Feb 20, 2020

I have a question about the scheduling of the task:
The old code used Timer.schedule(TimerTask,long,long) which schedules the task for repeated fixed-delay execution.
The new code uses ScheduledThreadPoolExecutor.scheduleAtFixedRate​(Runnable, long, long, TimeUnit) which schedules the task for repeated fixed-rate execution.

Now I think that scheduling at fixed rate would be the correct way as we want to reach 60 pulses per second. But my question is: Can this lead to problems if the work done per pulse takes longer than 16ms? The scheduleAtFixedRate does queue subsequent executions if the previous task takes too long. Couldn't this lead to an task queue overflow if the system is overloaded? Do we need to add protection for that scenario?

@dellgreen
Copy link
Contributor Author

dellgreen commented Feb 20, 2020

I may be wrong, but looking at the source code for both java.util.Timer.java and ScheduledThreadPoolExecutor.java, they both appear to grow their respective queues if needs be. So i don't think the proposed solution is any worse in that respect.

@littlefreaky
Copy link

littlefreaky commented Feb 20, 2020

Ok. Thanks for the clarification.

@openjdk
Copy link

openjdk bot commented May 1, 2020

@dellgreen This change now passes all automated pre-integration checks. When the change also fulfills all project specific requirements, type /integrate in a new comment to proceed. After integration, the commit message will be:

8176499: Dependence on java.util.Timer freezes screen when OS time resets backwards

Reviewed-by: jvos
  • If you would like to add a summary, use the /summary command.
  • To credit additional contributors, use the /contributor command.
  • To add additional solved issues, use the /solves command.

Since the source branch of this PR was last updated there have been 78 commits pushed to the master branch:

  • 2b9eb52: 8237504: Update copyright header for files modified in 2020
  • 3130fc8: 8198402: ToggleButton.setToggleGroup causes memory leak when button is removed via ToggleGroup.getToggles()
  • 8ad5805: 8191758: Match WebKit's font weight rendering with JavaFX
  • 66c3b38: 8227425: Add support for e-paper displays on i.MX6 devices
  • e30049f: 8242077: Add information about HTTP/2 and HttpClient usage in WebEngine
  • e0ffca3: 8242505: Some WebKit tests might fail because Microsoft libraries are not loaded
  • ceb3fce: 8087555: [ChoiceBox] uncontained value not shown
  • 818ac00: 8175358: Memory leak when moving MenuButton into another Scene
  • 91d4c8b: 8241737: TabPaneSkin memory leak on replacing selectionModel
  • 48476eb: 8241582: Infinite animation does not start from the end when started with a negative rate
  • ... and 68 more: https://git.openjdk.java.net/jfx/compare/b5e65ecc68ca550571e6b59911089c79855df6e3...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid automatic rebasing, please merge master into your branch, and then specify the current head hash when integrating, like this: /integrate 2b9eb52e4c2ed900fe9599fbe5e484fded764eba.

As you are not a known OpenJDK Author, an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@johanvos) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

@openjdk openjdk bot added the ready label May 1, 2020
@dellgreen
Copy link
Contributor Author

dellgreen commented May 1, 2020

/integrate
/summary Fixes a long standing issue in the MonocleTimer class as its dependent on the java.uti.Timer class which is dependent on system time which can cause UI freezes depending upon how far back in time the system clock is set by either a user or NTP.

@openjdk openjdk bot added the sponsor label May 1, 2020
@openjdk
Copy link

openjdk bot commented May 1, 2020

@dellgreen
Your change (at version 3c22d20) is now ready to be sponsored by a Committer.

@kevinrushforth
Copy link
Member

kevinrushforth commented May 1, 2020

@dellgreen /summary is a distinct command from /integrate -- you can't enter multiple commands in a single comment. /integrate should always be the last thing you do, since that indicates you are completely done with the PR and want to integrate it in its current state.

The /summary is going to be ignored, which is likely fine, but this is something to note for next time.

@dellgreen
Copy link
Contributor Author

dellgreen commented May 1, 2020

@kevinrushforth ok, thank you for the feedback

@johanvos
Copy link
Collaborator

johanvos commented May 1, 2020

/sponsor

@openjdk openjdk bot closed this May 1, 2020
@openjdk openjdk bot added integrated and removed sponsor ready labels May 1, 2020
@openjdk
Copy link

openjdk bot commented May 1, 2020

@johanvos @dellgreen The following commits have been pushed to master since your change was applied:

  • 2b9eb52: 8237504: Update copyright header for files modified in 2020
  • 3130fc8: 8198402: ToggleButton.setToggleGroup causes memory leak when button is removed via ToggleGroup.getToggles()
  • 8ad5805: 8191758: Match WebKit's font weight rendering with JavaFX
  • 66c3b38: 8227425: Add support for e-paper displays on i.MX6 devices
  • e30049f: 8242077: Add information about HTTP/2 and HttpClient usage in WebEngine
  • e0ffca3: 8242505: Some WebKit tests might fail because Microsoft libraries are not loaded
  • ceb3fce: 8087555: [ChoiceBox] uncontained value not shown
  • 818ac00: 8175358: Memory leak when moving MenuButton into another Scene
  • 91d4c8b: 8241737: TabPaneSkin memory leak on replacing selectionModel
  • 48476eb: 8241582: Infinite animation does not start from the end when started with a negative rate
  • dedf7cb: 8242490: Upgrade to gcc 9.2 on Linux
  • 5e9fb82: 8242577: Cell selection fails on iOS most of the times
  • 69e4266: 8242489: ChoiceBox: initially toggle not sync'ed to selection
  • 1d88180: 8243112: Skip failing test SVGTest.testSVGRenderingWithPattern
  • ec8608f: 8223298: SVG patterns are drawn wrong
  • e82046e: 8242530: [macos] Some audio files miss spectrum data when another audio file plays first
  • 7044cef: 8241476: Linux build warnings issued on gcc 9
  • 9d50c4c: Merge
  • 4d69a0d: 8241629: [macos10.15] Long startup delay playing media over https on Catalina
  • b1fdc45: 8242209: Increase web native thread stack size for x86 mode
  • e1cb191: 8240694: [macos 10.15] JavaFX Media hangs on some video files on Catalina
  • c154538: 8242106: [macos] Remove obsolete GlassView2D.m class
  • 3f663e3: 8240262: iOS refresh rate is capped to 30 Hz
  • 231879a: 8241710: NullPointerException while entering empty submenu with "arrow right"
  • 470c7d0: 8230809: HTMLEditor formatting lost when selecting all (CTRL-A)
  • fda015c: 8242167: ios keyboard handling
  • 844460b: 8242163: Android keyboard integration fails
  • 364c64a: 8241249: NPE in TabPaneSkin.perfromDrag
  • 418675a: 8236840: Memory leak when switching ButtonSkin
  • 247a65d: 8236971: [macos] Gestures handled incorrectly due to missing events
  • 560ef17: 8241455: Memory leak on replacing selection/focusModel
  • ef37669: Merge
  • 5906521: 8241370: Crash in JPEGImageLoader after fix for JDK-8212034
  • 159f651: 8240542: Switch FX build to use JDK 14 as boot JDK
  • 6d098fe: 8234959: FXMLLoader does not populate ENGINE_SCOPE Bindings with FILENAME and ARGV
  • d7f13f4: 8089828: RTL Orientation, the flag of a mnemonic is not placed under the mnemonic letter.
  • 9ecc107: 8240539: Upgrade gradle to version 6.3
  • f3a3ea0: 8234471: Canvas in webview displayed with wrong scale on Windows
  • d12e71c: 8241474: Build failing on Ubuntu 20.04
  • 2a7ab36: 8089134: [2D traversal, RTL] TraversalEngine only handles left/right key traversal correctly in RTL for top-level engine in ToolBar
  • 2aa8218: 8235480: Regression: [RTL] Arrow keys navigation doesn't respect TableView orientation
  • e9c6119: 8240692: Cleanup of the javafx property objects
  • f2bca9f: Merge
  • 90289a2: 8239107: Update libjpeg to version 9d
  • b81cf32: 8236259: MemoryLeak in ProgressIndicator
  • 9ea7f96: 8240832: Remove unused applecoreaudio.md third-party legal file
  • 0fc1420: Merge
  • 50e15fc: 8240466: AppJSCallback* apps launched by ModuleLauncherTest intermittently hang
  • e3026b9: 8240688: Remove the JavaBeanXxxPropertyBuilders constructors
  • f8c235b: 8240631: Create release notes for JavaFX 14
  • cfa1193: 8236685: [macOs] Remove obsolete file dialog subclasses
  • 6900d29: Merge
  • f25e8cf: 8212034: Potential memory leaks in jpegLoader.c in error case
  • b2ac76a: 8240451: JavaFX javadoc build fails with JDK 14
  • cf0bba6: 8240211: Stack overflow on Windows 32-bit can lead to crash
  • 337ed72: 8237926: Potential memory leak of model data in javafx.scene.control.ListView
  • 960f039: 8208761: Update constant collections to use the new immutable collections
  • 10c9528: 8240265: iOS: Unnecessary logging on pinch gestures
  • 4c132cd: 8237889: Update libxml2 to version 2.9.10
  • e91bec4: Merge
  • 20328b3: 8240218: IOS Webkit implementation broken
  • 4c82af8: 8236832: [macos 10.15] JavaFX Application hangs on video play on Cata…
  • 9cd6f79: 8196586: Remove use of deprecated finalize methods from javafx property objects
  • ef2f9ce: 8238755: allow to create static lib for javafx.media on linux
  • c3ee1a3: 8239822: Intermittent unit test failures in RegionCSSTest
  • 3150562: 8235772: Remove use of deprecated finalize method from PiscesRenderer and AbstractSurface
  • 4eaff0d: 8239109: Update SQLite to version 3.31.1
  • 66a8f49: Merge
  • d8e7f85: 8239454: LLIntData : invalid opcode returned for 16 and 32 bit wide instructions
  • 48ddd80: Merge
  • 35a01ca: 8228867: Fix mistakes in FX API docs
  • fde42da: Merge
  • e21fd1f: Merge
  • 443c845: Merge
  • 31e63de: Merge
  • 14c6938: 8236798: Enhance FX scripting support
  • bfb2d0e: Merge
  • 39f6127: Merge

Your commit was automatically rebased without conflicts.

Pushed as commit 1cae6a8.

@openjdk openjdk bot removed the rfr label May 1, 2020
@kevinrushforth kevinrushforth mentioned this pull request Jun 27, 2020
3 of 3 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.