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

8263363: Minor cleanup of Lanai code - unused code removal and comments correction #3357

Closed
wants to merge 6 commits into from

Conversation

@aghaisas
Copy link
Contributor

@aghaisas aghaisas commented Apr 6, 2021

Refer JBS for 3 issues that this PR addresses.
In addition, I have corrected an erroneous free() call in the same method I was cleaning up.


Progress

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

Issue

  • JDK-8263363: Minor cleanup of Lanai code - unused code removal and comments correction

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 3357

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

Using diff file

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

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Apr 6, 2021

👋 Welcome back aghaisas! 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 label Apr 6, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Apr 6, 2021

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

  • 2d
  • awt

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 awt labels Apr 6, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented Apr 6, 2021

// surfaces/contexts, so we should first invalidate the current
// Java-level context and flush the queue...
// getMTLConfigInfo() creates new MTLContext, so we should first
// invalidate the current Java-level context and flush the queue...
Copy link
Member

@mrserb mrserb Apr 7, 2021

Choose a reason for hiding this comment

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

The old discussion was related not only to the comment but to the invalidateCurrentContext, do we need to do it?

Copy link
Contributor Author

@aghaisas aghaisas Apr 8, 2021

Choose a reason for hiding this comment

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

This is the only place where we use MTLContext.invalidateCurrentContext() - which when processed in MTLRenderQueue - clears some native stuff and nulls out both mtlc and dstOps pointers maintained in MTLRenderQueue.m. I think, this will be important once we get rid of SET_SCRATCH_SURFACE under JDK-8263309.

Copy link
Member

@mrserb mrserb Apr 9, 2021

Choose a reason for hiding this comment

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

But why you need to invalidate context here? Why do you need "clears some native stuff and nulls out both mtlc and dstOps pointers maintained in MTLRenderQueue.m"?

In OGL the getCGLConfigInfo() change the state of the OGL state due to "makeCurrentContext", this is why we need to update the lava level state to "invalid", otherwise we will get a mismatch between the state in the native and java state.

Copy link
Contributor Author

@aghaisas aghaisas Apr 27, 2021

Choose a reason for hiding this comment

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

I see that MTLContext.invalidateCurrentContext() is not needed here. Also, as this is the only place it is used, we can remove the method altogether.

@@ -149,20 +149,6 @@ + (void) _getMTLConfigInfo: (NSMutableArray *)argValue {

NSAutoreleasePool* pool = [[NSAutoreleasePool alloc] init];


Copy link
Member

@mrserb mrserb Apr 7, 2021

Choose a reason for hiding this comment

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

Please also check how this function is called, looks like previously it was called as a selector+an array as a parameter, and then reworked as a performOnMainThreadWaiting+block, but it still use an array as a parameter. I think it is similar to JDK-8238075.

Copy link
Contributor Author

@aghaisas aghaisas Apr 8, 2021

Choose a reason for hiding this comment

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

Excellent point! Thanks for the pointer to the bug.
A lot of code in this file can be cleaned up. I will update the PR soon.

mrserb
mrserb approved these changes May 2, 2021
@openjdk
Copy link

@openjdk openjdk bot commented May 2, 2021

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

8263363: Minor cleanup of Lanai code - unused code removal and comments correction

Reviewed-by: serb

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

  • 5c083e8: 8266129: tools/jpackage/windows/WinInstallerIconTest.java hangs with fastdebug
  • 3e667cc: 8265356: need code example for getting canonical constructor of a Record
  • f86b70c: 8266328: C2: Remove InlineWarmCalls
  • 928d632: 8252237: C2: Call to compute_separating_interferences has wrong argument order
  • 50fa162: 8266389: ProblemList java/awt/Graphics2D/DrawString/DrawRotatedStringUsingRotatedFont.java on generic-all
  • dd05158: 8266155: Convert java.base to use Stream.toList()
  • c36c63a: 8260560: convert jdeps and jdeprscan tools to use Stream.toList()
  • 096e9e5: 8266318: Switch to macos prefix for macOS bundles
  • 0544a73: 8255227: java/net/httpclient/FlowAdapterPublisherTest.java intermittently failing with TestServer: start exception: java.io.IOException: Invalid preface
  • 48bb996: 8266220: keytool still prompt for store password on a password-less pkcs12 file if -storetype pkcs12 is specified
  • ... and 131 more: https://git.openjdk.java.net/jdk/compare/a85f6cbbaa816fafd348381326ed8ecac6453a5c...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.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

@openjdk openjdk bot added the ready label May 2, 2021
@aghaisas
Copy link
Contributor Author

@aghaisas aghaisas commented May 3, 2021

/integrate

@openjdk openjdk bot closed this May 3, 2021
@openjdk openjdk bot added integrated and removed ready rfr labels May 3, 2021
@openjdk
Copy link

@openjdk openjdk bot commented May 3, 2021

@aghaisas Since your change was applied there have been 143 commits pushed to the master branch:

  • 7e30130: 8266401: mark hotspot compiler/intrinsics/sha/cli tests which ignore VM flags
  • dedddd5: 8266248: Compilation failure in PLATFORM_API_MacOSX_MidiUtils.c with Xcode 12.5
  • 5c083e8: 8266129: tools/jpackage/windows/WinInstallerIconTest.java hangs with fastdebug
  • 3e667cc: 8265356: need code example for getting canonical constructor of a Record
  • f86b70c: 8266328: C2: Remove InlineWarmCalls
  • 928d632: 8252237: C2: Call to compute_separating_interferences has wrong argument order
  • 50fa162: 8266389: ProblemList java/awt/Graphics2D/DrawString/DrawRotatedStringUsingRotatedFont.java on generic-all
  • dd05158: 8266155: Convert java.base to use Stream.toList()
  • c36c63a: 8260560: convert jdeps and jdeprscan tools to use Stream.toList()
  • 096e9e5: 8266318: Switch to macos prefix for macOS bundles
  • ... and 133 more: https://git.openjdk.java.net/jdk/compare/a85f6cbbaa816fafd348381326ed8ecac6453a5c...master

Your commit was automatically rebased without conflicts.

Pushed as commit 8fa50eb.

💡 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
2 participants