Skip to content

Conversation

@jensli
Copy link
Contributor

@jensli jensli commented Aug 17, 2022

Prior to this change it was not clear from the documentation if callers of Platform#runLater must perform
any synchronisation to have writes of the calling thread be visible in the JavaFX Application
Thread. It is important to document either if callers can rely on runLater to do such synchronisation
internally, or to document is users CAN NOT rely on runLater for this.

This change documents that actions in a thread prior to submitting a runnable to
Platform#runLater happen-before actions performed by the runnable in the JavaFX
Application Thread.

runLater inherits the memory consistency effects of InvokeLaterDispatcher in most cases.
InvokeLaterDispatcher uses BlockingDeque internally. This change documents this
in the same way as it is documented by BlockingDeque.

Other implementations of runLater should have similar memory consistency effects.


Progress

  • Change must be properly reviewed (1 review required, with at least 1 Reviewer)
  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue

Issue

  • JDK-8293839: Documentation memory consistency effects of runLater

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jfx pull/872/head:pull/872
$ git checkout pull/872

Update a local copy of the PR:
$ git checkout pull/872
$ git pull https://git.openjdk.org/jfx pull/872/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 872

View PR using the GUI difftool:
$ git pr show -t 872

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jfx/pull/872.diff

Prior to this change it was not clear from the documentation if callers of Platform#runLater must perform
any synchronisation to have writes of the calling thread be visible in the JavaFX Application
Thread. It is important to document either if callers can rely on runLater to do such synchronisation
internally, or to document is users CAN NOT rely on runLater for this.

This change documents that actions in a thread prior to submitting a runnable to
Platform#runLater happen-before actions performed by the runnable in the JavaFX
Application Thread.

runLater inherits the memory consistency effects of InvokeLaterDispatcher in most cases. 
InvokeLaterDispatcher uses BlockingDeque internally. This change documents this
in the same way as it is documented by BlockingDeque.

Other implementations of runLater should have similar memory consistency effects.
@bridgekeeper bridgekeeper bot added the oca Needs verification of OCA signatory status label Aug 17, 2022
@bridgekeeper
Copy link

bridgekeeper bot commented Aug 17, 2022

Hi @jensli, welcome to this OpenJDK project and thanks for contributing!

We do not recognize you as Contributor and need to ensure you have signed the Oracle Contributor Agreement (OCA). If you have not signed the OCA, please follow the instructions. Please fill in your GitHub username in the "Username" field of the application. Once you have signed the OCA, please let us know by writing /signed in a comment in this pull request.

If you already are an OpenJDK Author, Committer or Reviewer, please click here to open a new issue so that we can record that fact. Please use "Add GitHub user jensli" as summary for the issue.

If you are contributing this work on behalf of your employer and your employer has signed the OCA, please let us know by writing /covered in a comment in this pull request.

@jensli
Copy link
Contributor Author

jensli commented Aug 17, 2022

/signed

@bridgekeeper bridgekeeper bot added the oca-verify Needs verification of OCA signatory status label Aug 17, 2022
@bridgekeeper
Copy link

bridgekeeper bot commented Aug 17, 2022

Thank you! Please allow for up to two weeks to process your OCA, although it is usually done within one to two business days. Also, please note that pull requests that are pending an OCA check will not usually be evaluated, so your patience is appreciated!

@bridgekeeper bridgekeeper bot removed oca Needs verification of OCA signatory status oca-verify Needs verification of OCA signatory status labels Aug 18, 2022
@kevinrushforth
Copy link
Member

@jensli I was going through the PRs that are not marked as rfr (ready for review), and spotted this one. There are two needed steps before this will be marked as ready:

  1. You need to file a bug report
  2. You need to fix the whitespace error pointed out by jcheck in the code diffs

See CONTRIBUTING.md for more information.

@jensli jensli changed the title Document memory consistency effects of runLater 9073947: Document memory consistency effects of runLater Sep 14, 2022
@jensli jensli changed the title 9073947: Document memory consistency effects of runLater 9073947: Documentation memory consistency effects of runLater Sep 14, 2022
@jensli
Copy link
Contributor Author

jensli commented Sep 14, 2022

/summary 8293839: Document memory consistency effects of runLater

@openjdk
Copy link

openjdk bot commented Sep 14, 2022

@jensli Setting summary to 9073947: Document memory consistency effects of runLater

@jensli
Copy link
Contributor Author

jensli commented Sep 14, 2022

Thanks for the help, @kevinrushforth. I was not prepared for all the rigour of the OpenJDK contribution process. This has been a perfect opportunity for me to learn about it.

When my bug report submission is accepted then hopefully all formal requirements will be met. (Maybe also the internal review bug ID have to be updated to the proper bug ID.)

@jensli jensli changed the title 9073947: Documentation memory consistency effects of runLater 8293839: Documentation memory consistency effects of runLater Sep 15, 2022
@openjdk openjdk bot added the rfr Ready for review label Sep 15, 2022
@mlbridge
Copy link

mlbridge bot commented Sep 15, 2022

Webrevs

@jensli
Copy link
Contributor Author

jensli commented Sep 15, 2022

/summary 8293839: Document memory consistency effects of runLater

@openjdk
Copy link

openjdk bot commented Sep 15, 2022

@jensli Updating existing summary to 8293839: Document memory consistency effects of runLater

@jensli
Copy link
Contributor Author

jensli commented Sep 15, 2022

@kevinrushforth This is now finished. Can you review?

@kevinrushforth
Copy link
Member

kevinrushforth commented Sep 15, 2022

This is now ready to review, so I'll take a look. One thing to note, though, is that the /summary is an optional extra line of information that gets recorded into a commit. Most of the time it is unused, but if it is used, it should not be a copy of the title. I recommend removing it with /summary in a comment by itself with no other text.

@kevinrushforth kevinrushforth self-requested a review September 15, 2022 11:21
Copy link
Member

@kevinrushforth kevinrushforth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The added description looks good. I left one suggestion on using code style for the named argument.

@jensli
Copy link
Contributor Author

jensli commented Sep 15, 2022

One thing to note, though, is that the /summary is an optional extra line of information that gets recorded into a commit.

Ah, okay, thanks.

Do I remove the summary by issuing and empty /summary?

Will the commit message of the squshed commit be taken from the PR description?

@kevinrushforth
Copy link
Member

Do I remove the summary by issuing and empty /summary?

Will the commit message of the squshed commit be taken from the PR description?

Yes to both questions.

@jensli
Copy link
Contributor Author

jensli commented Sep 15, 2022

/summary

@openjdk
Copy link

openjdk bot commented Sep 15, 2022

@jensli Removing existing summary

@jensli
Copy link
Contributor Author

jensli commented Sep 15, 2022

@kevinrushforth I think this is now finished.

Thanks for teaching me about the OpenJDK contribution process! :)

Copy link
Member

@kevinrushforth kevinrushforth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good.

@openjdk
Copy link

openjdk bot commented Sep 16, 2022

@jensli This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8293839: Documentation memory consistency effects of runLater

Reviewed-by: kcr

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 28 new commits pushed to the master branch:

  • a27840e: 8293368: GitHub Workflows security hardening
  • 5e4552d: 8089280: horizontal scrollbar should never become visible in TableView with constrained resize policy
  • cef583e: 8293214: Add support for Linux/LoongArch64
  • 27f1905: 8279640: ListView with null SelectionModel/FocusModel throws NPE
  • 205b721: 8293615: Create release notes for JavaFX 19
  • 28f8fa9: 8293375: add_definitions USE_SYSTEM_MALLOC when USE_SYSTEM_MALLOC is ON
  • 160be03: 8293171: Minor typographical errors in JavaDoc javafx.scene.control.ScrollPane.java
  • 77f1995: 8293338: Enable manually triggering the GitHub workflow
  • 2baa10e: 8087673: [TableView] TableView and TreeTableView menu button overlaps columns when using a constrained resize policy.
  • d1a7ebd: 8279514: NPE on clearing value of IntegerSpinnerValueFactory
  • ... and 18 more: https://git.openjdk.org/jfx/compare/eaddb0fbeeb99900636f9704758f6c004860ff9a...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@kevinrushforth) 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 Ready to be integrated label Sep 16, 2022
@kevinrushforth
Copy link
Member

@jensli This is now ready for you to /integrate.

@jensli
Copy link
Contributor Author

jensli commented Sep 20, 2022

/integrate

@openjdk openjdk bot added the sponsor Ready to sponsor label Sep 20, 2022
@openjdk
Copy link

openjdk bot commented Sep 20, 2022

@jensli
Your change (at version d935dc1) is now ready to be sponsored by a Committer.

@nlisker
Copy link
Collaborator

nlisker commented Sep 20, 2022

/sponsor

@openjdk
Copy link

openjdk bot commented Sep 20, 2022

Going to push as commit 7c6a54d.
Since your change was applied there have been 28 commits pushed to the master branch:

  • a27840e: 8293368: GitHub Workflows security hardening
  • 5e4552d: 8089280: horizontal scrollbar should never become visible in TableView with constrained resize policy
  • cef583e: 8293214: Add support for Linux/LoongArch64
  • 27f1905: 8279640: ListView with null SelectionModel/FocusModel throws NPE
  • 205b721: 8293615: Create release notes for JavaFX 19
  • 28f8fa9: 8293375: add_definitions USE_SYSTEM_MALLOC when USE_SYSTEM_MALLOC is ON
  • 160be03: 8293171: Minor typographical errors in JavaDoc javafx.scene.control.ScrollPane.java
  • 77f1995: 8293338: Enable manually triggering the GitHub workflow
  • 2baa10e: 8087673: [TableView] TableView and TreeTableView menu button overlaps columns when using a constrained resize policy.
  • d1a7ebd: 8279514: NPE on clearing value of IntegerSpinnerValueFactory
  • ... and 18 more: https://git.openjdk.org/jfx/compare/eaddb0fbeeb99900636f9704758f6c004860ff9a...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot added the integrated Pull request has been integrated label Sep 20, 2022
@openjdk openjdk bot closed this Sep 20, 2022
@openjdk openjdk bot removed ready Ready to be integrated rfr Ready for review sponsor Ready to sponsor labels Sep 20, 2022
@openjdk
Copy link

openjdk bot commented Sep 20, 2022

@nlisker @jensli Pushed as commit 7c6a54d.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

integrated Pull request has been integrated

Development

Successfully merging this pull request may close these issues.

3 participants