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

8272756: Remove unnecessary explicit initialization of volatile variables in java.desktop #5197

Closed
wants to merge 3 commits into from

Conversation

stsypanov
Copy link
Contributor

@stsypanov stsypanov commented Aug 20, 2021

This is a continuation of

As mentioned in JDK-6736490:

An explicit initialization of a volatile class instance variable, such as private volatile Object = null; or private volatile int scale = 0; is unnecessary since the Java spec automatically initializes objects to null and primitive type short, int, long, float and double to 0 and boolean to false. Explicit initialization of volatile variable to a value the same as the default implicit initialized value results in an unnecessary store and membar operation.


Progress

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

Issue

  • JDK-8272756: Remove unnecessary explicit initialization of volatile variables in java.desktop

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/5197/head:pull/5197
$ git checkout pull/5197

Update a local copy of the PR:
$ git checkout pull/5197
$ git pull https://git.openjdk.java.net/jdk pull/5197/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 5197

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/5197.diff

@bridgekeeper
Copy link

bridgekeeper bot commented Aug 20, 2021

👋 Welcome back stsypanov! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk openjdk bot added the rfr Pull request is ready for review label Aug 20, 2021
@openjdk
Copy link

openjdk bot commented Aug 20, 2021

@stsypanov The following labels will be automatically applied to this pull request:

  • 2d
  • awt
  • i18n
  • sound
  • swing

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

@openjdk openjdk bot added 2d client-libs-dev@openjdk.org swing client-libs-dev@openjdk.org sound client-libs-dev@openjdk.org i18n i18n-dev@openjdk.org awt client-libs-dev@openjdk.org labels Aug 20, 2021
@mlbridge
Copy link

mlbridge bot commented Aug 20, 2021

Webrevs


private static void initializeEssentialTags() {
Set<Integer> tags = essentialTags;
if (tags == null) {
essentialTags = tags = Set.of(
essentialTags = Set.of(
Copy link
Member

@mrserb mrserb Aug 20, 2021

Choose a reason for hiding this comment

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

What is the purpose of the local tags here?

Copy link
Contributor Author

@stsypanov stsypanov Aug 23, 2021

Choose a reason for hiding this comment

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

Looks like there's no purpose, tags is not used after assignment

Copy link
Member

@aivanov-jdk aivanov-jdk Aug 23, 2021

Choose a reason for hiding this comment

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

Maybe it's kind of a protection from a race. Yet it's possible either way: another thread could see essentialTags == null before this one initialises the field.

Copy link
Member

@mrserb mrserb Aug 23, 2021

Choose a reason for hiding this comment

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

I think we can drop it completely.

Copy link
Member

@aivanov-jdk aivanov-jdk Aug 23, 2021

Choose a reason for hiding this comment

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

Agree.

Copy link
Member

@mrserb mrserb Sep 26, 2021

Choose a reason for hiding this comment

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

I still suggest to remove the local tag completely.

Copy link
Member

@mrserb mrserb Oct 8, 2021

Choose a reason for hiding this comment

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

@jayathirthrao @stsypanov Looks like this was not addressed? It will be good to include it in some future cleanup.

Copy link
Member

@aivanov-jdk aivanov-jdk Oct 8, 2021

Choose a reason for hiding this comment

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

As far as I can see, it was. Line 64:

            essentialTags = Set.of(

And it shows as such in the diff of the commit.

Copy link
Member

@mrserb mrserb Oct 8, 2021

Choose a reason for hiding this comment

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

That was the usage of the local var, the current thread was about the local var itself, it is still there.

Copy link
Member

@aivanov-jdk aivanov-jdk Oct 8, 2021

Choose a reason for hiding this comment

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

Good catch!
I missed that the local variable is still there and now it's unused, so an IDE should issue a warning about it.

@turbanoff
Copy link
Member

turbanoff commented Sep 20, 2021

As I can see there are a few more zero-initializations of volatiles. Are they intentionally skipped?

  1. sun.lwawt.macosx.CPlatformEmbeddedFrame#screenX
  2. sun.lwawt.macosx.CPlatformEmbeddedFrame#screenY
  3. sun.lwawt.macosx.CWarningWindow#currentIcon
  4. com.sun.media.sound.SoftSynthesizer.WeakAudioStream#silent_samples

@stsypanov
Copy link
Contributor Author

stsypanov commented Sep 21, 2021

@turbanoff nice catch! I've missed them in my original commit, no intention here. I've included them either.

@stsypanov
Copy link
Contributor Author

stsypanov commented Sep 26, 2021

/integrate

@openjdk
Copy link

openjdk bot commented Sep 26, 2021

@stsypanov This PR has not yet been marked as ready for integration.

@openjdk
Copy link

openjdk bot commented Oct 8, 2021

⚠️ @stsypanov the full name on your profile does not match the author name in this pull requests' HEAD commit. If this pull request gets integrated then the author name from this pull requests' HEAD commit will be used for the resulting commit. If you wish to push a new commit with a different author name, then please run the following commands in a local repository of your personal fork:

$ git checkout 8272756
$ git commit -c user.name='Preferred Full Name' --allow-empty -m 'Update full name'
$ git push

@openjdk
Copy link

openjdk bot commented Oct 8, 2021

@stsypanov 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:

8272756: Remove unnecessary explicit initialization of volatile variables in java.desktop

Reviewed-by: jdv, aivanov

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 153 new commits pushed to the master branch:

  • 36b89a1: 8274785: ciReplay: Potential crash due to uninitialized Compile::_ilt variable
  • 2aacd42: 8274145: C2: condition incorrectly made redundant with dominating main loop exit condition
  • 6364719: 8274004: Change 'nonleaf' rank name
  • b60837a: 8272586: emit abstract machine code in hs-err logs
  • 97ea9dd: 8274840: Update OS detection code to recognize Windows 11
  • 8de7763: 8273152: Refactor CDS FileMapHeader loading code
  • 920e707: 8274920: ProblemList 2 VectorAPI tests failing due to "assert(!vbox->is_Phi()) failed"
  • 7de2cf8: 8273910: Redundant condition and assignment in java.net.URI
  • 8ca0846: 8274407: (tz) Update Timezone Data to 2021c
  • 4ab274a: 8274858: Remove unused dictionary_classes_do functions
  • ... and 143 more: https://git.openjdk.java.net/jdk/compare/4838a2ca7c8e75b95c1c68ada7523e2a94815f45...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 (@jayathirthrao, @aivanov-jdk) 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 Pull request is ready to be integrated label Oct 8, 2021
@stsypanov
Copy link
Contributor Author

stsypanov commented Oct 8, 2021

/integrate

@openjdk openjdk bot added the sponsor Pull request is ready to be sponsored label Oct 8, 2021
@openjdk
Copy link

openjdk bot commented Oct 8, 2021

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

@jayathirthrao
Copy link
Member

jayathirthrao commented Oct 8, 2021

/sponsor

@openjdk
Copy link

openjdk bot commented Oct 8, 2021

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

  • 36b89a1: 8274785: ciReplay: Potential crash due to uninitialized Compile::_ilt variable
  • 2aacd42: 8274145: C2: condition incorrectly made redundant with dominating main loop exit condition
  • 6364719: 8274004: Change 'nonleaf' rank name
  • b60837a: 8272586: emit abstract machine code in hs-err logs
  • 97ea9dd: 8274840: Update OS detection code to recognize Windows 11
  • 8de7763: 8273152: Refactor CDS FileMapHeader loading code
  • 920e707: 8274920: ProblemList 2 VectorAPI tests failing due to "assert(!vbox->is_Phi()) failed"
  • 7de2cf8: 8273910: Redundant condition and assignment in java.net.URI
  • 8ca0846: 8274407: (tz) Update Timezone Data to 2021c
  • 4ab274a: 8274858: Remove unused dictionary_classes_do functions
  • ... and 143 more: https://git.openjdk.java.net/jdk/compare/4838a2ca7c8e75b95c1c68ada7523e2a94815f45...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot closed this Oct 8, 2021
@openjdk openjdk bot added integrated Pull request has been integrated and removed ready Pull request is ready to be integrated rfr Pull request is ready for review sponsor Pull request is ready to be sponsored labels Oct 8, 2021
@openjdk
Copy link

openjdk bot commented Oct 8, 2021

@jayathirthrao @stsypanov Pushed as commit ccbce10.

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

@stsypanov stsypanov deleted the 8272756 branch Oct 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2d client-libs-dev@openjdk.org awt client-libs-dev@openjdk.org i18n i18n-dev@openjdk.org integrated Pull request has been integrated sound client-libs-dev@openjdk.org swing client-libs-dev@openjdk.org
5 participants