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

8259651: [macOS] Replace JNF_COCOA_ENTER/EXIT macros #2056

Closed
wants to merge 3 commits into from
Closed

8259651: [macOS] Replace JNF_COCOA_ENTER/EXIT macros #2056

wants to merge 3 commits into from

Conversation

@prrace
Copy link
Contributor

@prrace prrace commented Jan 12, 2021

Most of the changes here are simply
JNF_COCOA_ENTER -> JNI_COCOA_ENTER
JNF_COCOA_EXIT -> JNI_COCOA_EXIT

These new macros are defined in JNIUtilities.h and handle the auto release and on exit catch any NSException.
Unlike the JNF code, JNI exceptions don't have to be extracted from the NSException.

So calls to JNFException are also removed and in most cases they are just directly using one of the JNU_*
defined exceptions since we are in a native nethod and can return control directly to Java.

JNIUtilities has just two macros for cases where we need to accompany it with an NSException because
we aren't in the immediate body of a JNI method.

JNIUtilities also has a macro JNI_COCOA_EXIT_WITH_ACTION
This is used by a macro in QuartzSurfaceData.m to re-implement a pre-existing macro
JNF_COCOA_RENDERER_EXIT as JNI_COCOA_RENDERER_EXIT.
This is used in a few places to ensure that we call FinishSurface on the Quartz surface.

This already passed all our automated tests, although I'm re-running since I needed to merge to the
current repo state.


Progress

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

Issue

  • JDK-8259651: [macOS] Replace JNF_COCOA_ENTER/EXIT macros

Reviewers

Download

$ git fetch https://git.openjdk.java.net/jdk pull/2056/head:pull/2056
$ git checkout pull/2056

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Jan 12, 2021

👋 Welcome back prr! 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
Copy link

@openjdk openjdk bot commented Jan 12, 2021

@prrace The following label will be automatically applied to this pull request:

  • awt

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

@openjdk openjdk bot added the awt label Jan 12, 2021
@openjdk openjdk bot added the rfr label Jan 13, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented Jan 13, 2021

Webrevs

[NSException raise:@"Java Exception" reason:@"Java OutOfMemoryException" userInfo:nil]

#define JNI_COCOA_THROW_RUNTIME_EXCEPTION(env, msg) \
JNU_ThrowByName(env, "java/lang/RuntimeException", msg); \
Copy link
Member

@mrserb mrserb Jan 13, 2021

Can we guarantee that before the next java code executed later we clean this exception?

BTW I still think the exceptions wrapping will be a better thing. If it will be caught by our catch block like in JNI_COCOA_EXIT we can unwrap it, otherwise, the java code could be executed safely since we clear an exception during wrapping.

Copy link
Contributor Author

@prrace prrace Jan 13, 2021

Since the next line throws NSException everything before this change and after this change should cause control to reach the COCOA_EXIT macro. I am not aware of anything in between. So this should be OK. The previous JNF code (as I discussed in some earlier review / thread) seems to be internally responsible for not doing things properly as I had found that its exception handling code actually made calls that aren't allowed with the exception pending. And this should not happen any more.

Copy link
Member

@mrserb mrserb Jan 13, 2021

Since the next line throws NSException everything before this change and after this change should cause control to reach the COCOA_EXIT macro.

It does not, the JNI_COCOA_EXIT macro is executed on a different thread than JNI_COCOA_THROW_RUNTIME_EXCEPTION (in both times where this macro is used)

Copy link
Contributor Author

@prrace prrace Jan 13, 2021

You are correct. This is running in a performOnMainThreadWaiting:NO block.
So the JNI call likely already returned to Java before that block gets run on the AppKit thread.
But this seems to be the same as before and there was probably never any point to raising the JDK exception
I think in this case maybe what I should is update the macro to skip throwing the Java Exception if
it is being raised on the AppKit thread. No caller that I can see is expecting a RuntimeException for these failed actions so this seems to be very much an implementation decision. I am not even sure the code should be doing this. Rightly or wrongly GraphicsDevice.setFullScreenWindow(Window) declares no exceptions or possibility of failure.

Copy link
Member

@mrserb mrserb Jan 13, 2021

It is always better to wrap the java exception and always clear after each callJavaMethod, so the next callJavaMethod will not be called when the java exception was raised. At the same time in the place where we catch the nsexception we can check does it have a java exception or not -> so we can unwrap and raise again the java exception just before return to the java code, or such exception in the NSApplication global catch block(including currently missing NSApplication#reportException).

Copy link
Contributor Author

@prrace prrace Jan 14, 2021

It is one way of doing it but it is much more heavyweight as it requires a carrier for the exception
and so goes against the approach here which is to be very lightweight in the sense of avoiding
creating classes - which so far as been the case.
And there is no need to wrap it. If its there, it will be just be passed on up to Java without any
work on our part.
And as for clearing, that's what I was doing before and that made sense if the intention is to carry on.
Since now we are always raising NSException there that next Java call shouldn't happen as we always
should jump out.

[JNFException raise:[ThreadUtilities getJNIEnv]
as:kRuntimeException
reason:"Failed to enter full screen."];
[NSException raise:@"Java Exception" reason:@"Failed to enter full screen." userInfo:nil];
Copy link
Contributor Author

@prrace prrace Jan 14, 2021

Since this code is lexically inside a block executed on the AppKit thread it makes no sense to raise a Java Exception.

JNU_ThrowOutOfMemoryError(env, msg); \
} \
[NSException raise:@"Java Exception" reason:@"Java OutOfMemoryException" userInfo:nil]

Copy link
Contributor Author

@prrace prrace Jan 14, 2021

This macro is needed only in this file.

mrserb
mrserb approved these changes Jan 15, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Jan 15, 2021

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

8259651: [macOS] Replace JNF_COCOA_ENTER/EXIT macros

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

  • 2c8e337: 8259622: TreeMap.computeIfAbsent deviates from spec
  • d701bab: Merge
  • 4307fa6: 8253505: JFR: onFlush invoked out of order with a sorted event stream
  • 0148adf: 8255120: C2: assert(outer->outcnt() >= phis + 2 && outer->outcnt() <= phis + 2 + stores + 1) failed: only phis
  • 90960c5: 8252657: JVMTI agent is not unloaded when Agent_OnAttach is failed
  • e3b548a: 8257736: InputStream from BodyPublishers.ofInputStream() leaks when IOE happens
  • 978bed6: 8259522: Apply java.io.Serial annotations in java.desktop
  • bf28f92: 8259713: Fix comments about ResetNoHandleMark in deoptimization
  • 4f881ba: 8258652: Assert in JvmtiThreadState::cur_stack_depth() can noticeably slow down debugging single stepping
  • d18d26e: 8259350: Add some internal debugging APIs to the debug agent
  • ... and 54 more: https://git.openjdk.java.net/jdk/compare/5f7ccce0c03b2b814c0bae132d359d9903708496...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 Jan 15, 2021
@prrace
Copy link
Contributor Author

@prrace prrace commented Jan 15, 2021

/integrate

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

@openjdk openjdk bot commented Jan 15, 2021

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

  • 360c722: 8259729: Missed JNFInstanceOf -> IsInstanceOf conversion
  • b78cd63: 8259846: [BACKOUT] JDK-8259278 Optimize Vector API slice and unslice operations
  • eb7fa00: 8259216: javadoc omits method receiver for any nested type annotation
  • bcf20a0: 8259777: Incorrect predication condition generated by ADLC
  • bbac91a: 8257959: Add gtest run with -XX:+UseLargePages
  • 707bce0: 8257212: (bf spec) Clarify byte order of the buffer returned by CharBuffer.subsequence(int,int)
  • 0ec2c96: 8259820: JShell does not handle -source 8 properly
  • b01a15e: 8258884: [TEST_BUG] Convert applet-based test open/test/jdk/javax/swing/JMenuItem/8031573/bug8031573.java to a regular java test
  • 6d4a593: 8259627: Potential memory leaks in JVMTI after JDK-8227745
  • 2c8e337: 8259622: TreeMap.computeIfAbsent deviates from spec
  • ... and 63 more: https://git.openjdk.java.net/jdk/compare/5f7ccce0c03b2b814c0bae132d359d9903708496...master

Your commit was automatically rebased without conflicts.

Pushed as commit 5855d52.

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

@prrace prrace deleted the jnf_jni_cocoa branch Feb 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
2 participants