Re: RFR: 8257988: Remove JNF dependency from libsaproc/MacosxDebuggerLocal.m [v2]
On Wed, 3 Feb 2021 20:17:03 GMT, Phil Race wrote: >> This removes the JNF dependency from the jdk.hotspot.agent module. >> The macro expansions are the same as already used in the Java desktop module >> - which actually uses a macro >> still but there there are hundreds of uses. >> The function of this macro code is to prevent NSExceptions escaping to Java >> and also to drain the auto-release pool. >> What test group would be good for verifying this change ? > > Phil Race has updated the pull request incrementally with one additional > commit since the last revision: > > 8257988: Remove JNF dependency from libsaproc/MacosxDebuggerLocal.m Marked as reviewed by cjplummer (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/2304
Re: RFR: 8257988: Remove JNF dependency from libsaproc/MacosxDebuggerLocal.m [v2]
On Tue, 2 Feb 2021 20:27:17 GMT, Chris Plummer wrote:
>> Phil Race has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> 8257988: Remove JNF dependency from libsaproc/MacosxDebuggerLocal.m
>
> src/jdk.hotspot.agent/macosx/native/libsaproc/MacosxDebuggerLocal.m line 294:
>
>> 292:
>> 293: NSAutoreleasePool *pool = [[NSAutoreleasePool alloc] init];
>> 294: @try {
>
> Although there are only 3 places where the `JNF_COCOA_ENTER/EXIT` macros are
> used, it seems it would still be worth taking the same approach you did for
> `java.desktop` and add the replacement macros instead of inlining them. So
> just copy what you added to
> `src/java.desktop/macosx/native/libosxapp/JNIUtilities.h`, and replace
> `JNF_COCOA_ENTER` with `JNI_COCOA_ENTER/EXIT`. Otherwise the
> `JNF_COCOA_ENTER/EXIT` changes look fine to me, but I'm just basing this on a
> comparison with what you've done with `java.desktop`. I'm no expert in this
> area.
OK .. I don't really mind either way and if this helps gets it pushed .. so
I've updated.
> src/jdk.hotspot.agent/macosx/native/libsaproc/MacosxDebuggerLocal.m line 296:
>
>> 294: @try {
>> 295:
>> 296: NSString *symbolNameString = JavaStringToNSString(env, symbolName);
>
> Is there a reason why `java.desktop` continues to use `JNFJavaToNSString`? I
> was looking to see how this was handled in other places, but I couldn't find
> an example where `JNFJavaToNSString` was converted to call a newly
> implemented `JavaStringToNSString`.
As Magnus said that is in progress. Hoping it will be pushed very soon.
-
PR: https://git.openjdk.java.net/jdk/pull/2304
Re: RFR: 8257988: Remove JNF dependency from libsaproc/MacosxDebuggerLocal.m [v2]
> This removes the JNF dependency from the jdk.hotspot.agent module. > The macro expansions are the same as already used in the Java desktop module > - which actually uses a macro > still but there there are hundreds of uses. > The function of this macro code is to prevent NSExceptions escaping to Java > and also to drain the auto-release pool. > What test group would be good for verifying this change ? Phil Race has updated the pull request incrementally with one additional commit since the last revision: 8257988: Remove JNF dependency from libsaproc/MacosxDebuggerLocal.m - Changes: - all: https://git.openjdk.java.net/jdk/pull/2304/files - new: https://git.openjdk.java.net/jdk/pull/2304/files/9919a02c..d7ed0158 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2304&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2304&range=00-01 Stats: 39 lines in 1 file changed: 18 ins; 15 del; 6 mod Patch: https://git.openjdk.java.net/jdk/pull/2304.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2304/head:pull/2304 PR: https://git.openjdk.java.net/jdk/pull/2304
Re: RFR: 8257988: Remove JNF dependency from libsaproc/MacosxDebuggerLocal.m
On 2021-02-02 21:47, Chris Plummer wrote: Is there a reason why `java.desktop` continues to use `JNFJavaToNSString`? I was looking to see how this was handled in other places, but I couldn't find an example where `JNFJavaToNSString` was converted to call a newly implemented `JavaStringToNSString`. That conversion is handled in https://github.com/openjdk/jdk/pull/2305. /Magnus
Re: RFR: 8257988: Remove JNF dependency from libsaproc/MacosxDebuggerLocal.m
On Thu, 28 Jan 2021 22:40:57 GMT, Phil Race wrote:
> This removes the JNF dependency from the jdk.hotspot.agent module.
> The macro expansions are the same as already used in the Java desktop module
> - which actually uses a macro
> still but there there are hundreds of uses.
> The function of this macro code is to prevent NSExceptions escaping to Java
> and also to drain the auto-release pool.
> What test group would be good for verifying this change ?
src/jdk.hotspot.agent/macosx/native/libsaproc/MacosxDebuggerLocal.m line 294:
> 292:
> 293: NSAutoreleasePool *pool = [[NSAutoreleasePool alloc] init];
> 294: @try {
Although there are only 3 places where the `JNF_COCOA_ENTER/EXIT` macros are
used, it seems it would still be worth taking the same approach you did for
`java.desktop` and add the replacement macros instead of inlining them. So just
copy what you added to
`src/java.desktop/macosx/native/libosxapp/JNIUtilities.h`, and replace
`JNF_COCOA_ENTER` with `JNI_COCOA_ENTER/EXIT`. Otherwise the
`JNF_COCOA_ENTER/EXIT` changes look fine to me, but I'm just basing this on a
comparison with what you've done with `java.desktop`. I'm no expert in this
area.
src/jdk.hotspot.agent/macosx/native/libsaproc/MacosxDebuggerLocal.m line 296:
> 294: @try {
> 295:
> 296: NSString *symbolNameString = JavaStringToNSString(env, symbolName);
Is there a reason why `java.desktop` continues to use `JNFJavaToNSString`? I
was looking to see how this was handled in other places, but I couldn't find an
example where `JNFJavaToNSString` was converted to call a newly implemented
`JavaStringToNSString`.
-
PR: https://git.openjdk.java.net/jdk/pull/2304
Re: RFR: 8257988: Remove JNF dependency from libsaproc/MacosxDebuggerLocal.m
On Fri, 29 Jan 2021 17:22:49 GMT, Phil Race wrote: >> Build changes look good. > >> Is it because they are not in a place that can be accessed from this file? > Right 99% of JNF usage was the desktop module many, many files, 1300 > references .. the entire rest of the JDK had just 3 files each in a different > module ! It did not seem worth creating a JDK-wide platform-specific library > to support this. > For testing you want test/jdk/sun/tools/jhsdb/ and > test/hotspot/jtreg/serviceability/sa These tests passed with my changes - PR: https://git.openjdk.java.net/jdk/pull/2304
Re: RFR: 8257988: Remove JNF dependency from libsaproc/MacosxDebuggerLocal.m
On Fri, 29 Jan 2021 10:01:42 GMT, Magnus Ihse Bursie wrote: >> This removes the JNF dependency from the jdk.hotspot.agent module. >> The macro expansions are the same as already used in the Java desktop module >> - which actually uses a macro >> still but there there are hundreds of uses. >> The function of this macro code is to prevent NSExceptions escaping to Java >> and also to drain the auto-release pool. >> What test group would be good for verifying this change ? > > Build changes look good. > Is it because they are not in a place that can be accessed from this file? Right 99% of JNF usage was the desktop module many, many files, 1300 references .. the entire rest of the JDK had just 3 files each in a different module ! It did not seem worth creating a JDK-wide platform-specific library to support this. - PR: https://git.openjdk.java.net/jdk/pull/2304
Re: RFR: 8257988: Remove JNF dependency from libsaproc/MacosxDebuggerLocal.m
On Thu, 28 Jan 2021 22:40:57 GMT, Phil Race wrote: > This removes the JNF dependency from the jdk.hotspot.agent module. > The macro expansions are the same as already used in the Java desktop module > - which actually uses a macro > still but there there are hundreds of uses. > The function of this macro code is to prevent NSExceptions escaping to Java > and also to drain the auto-release pool. > What test group would be good for verifying this change ? Build changes look good. - Marked as reviewed by ihse (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/2304
Re: RFR: 8257988: Remove JNF dependency from libsaproc/MacosxDebuggerLocal.m
On Fri, 29 Jan 2021 00:15:56 GMT, Chris Plummer wrote: >> This removes the JNF dependency from the jdk.hotspot.agent module. >> The macro expansions are the same as already used in the Java desktop module >> - which actually uses a macro >> still but there there are hundreds of uses. >> The function of this macro code is to prevent NSExceptions escaping to Java >> and also to drain the auto-release pool. >> What test group would be good for verifying this change ? > > For testing you want `test/jdk/sun/tools/jhsdb/` and > `test/hotspot/jtreg/serviceability/sa` I'm doubtful you'll find anyone on serviceability-dev that understands JNF and the implications it has on MacosxDebuggerLocal.m. Although I've done a lot of work in this file myself recently, it's all bee sans any knowledge of JNF, Cocoa, or Objective C. You might be better off asking reviewers that looked at other recent PRs to remove JNF usage. However, having looked through [JDK-8257852](https://bugs.openjdk.java.net/browse/JDK-8257852) and from there finding [JDK-8259651](https://bugs.openjdk.java.net/browse/JDK-8259651), I'm wondering why you didn't just replace JNF_COCOA_ENTER/EXIT with the new JNI_COCOA_ENTER/EXIT in this PR also? Is it because they are not in a place that can be accessed from this file? - PR: https://git.openjdk.java.net/jdk/pull/2304
Re: RFR: 8257988: Remove JNF dependency from libsaproc/MacosxDebuggerLocal.m
On Thu, 28 Jan 2021 22:40:57 GMT, Phil Race wrote: > This removes the JNF dependency from the jdk.hotspot.agent module. > The macro expansions are the same as already used in the Java desktop module > - which actually uses a macro > still but there there are hundreds of uses. > The function of this macro code is to prevent NSExceptions escaping to Java > and also to drain the auto-release pool. > What test group would be good for verifying this change ? For testing you want `test/jdk/sun/tools/jhsdb/` and `test/hotspot/jtreg/serviceability/sa` - PR: https://git.openjdk.java.net/jdk/pull/2304
RFR: 8257988: Remove JNF dependency from libsaproc/MacosxDebuggerLocal.m
This removes the JNF dependency from the jdk.hotspot.agent module. The macro expansions are the same as already used in the Java desktop module - which actually uses a macro still but there there are hundreds of uses. The function of this macro code is to prevent NSExceptions escaping to Java and also to drain the auto-release pool. What test group would be good for verifying this change ? - Commit messages: - 8257988: Remove JNF dependency from libsaproc/MacosxDebuggerLocal.m - 8257988: Remove JNF dependency from libsaproc/MacosxDebuggerLocal.m Changes: https://git.openjdk.java.net/jdk/pull/2304/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=2304&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8257988 Stats: 42 lines in 2 files changed: 32 ins; 2 del; 8 mod Patch: https://git.openjdk.java.net/jdk/pull/2304.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2304/head:pull/2304 PR: https://git.openjdk.java.net/jdk/pull/2304
