RFR: 8258836: JNI local refs exceed capacity getDiagnosticCommandInfo [v4]

[Severin Gehwolf]

On Thu, 21 Jan 2021 09:06:00 GMT, Aleksey Shipilev <shade at openjdk.org> wrote:

>> Severin Gehwolf has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains six additional commits since the last revision:
>> 
>>  - Merge branch 'master' into JDK-8258836-check-jni-mbeanserver
>>  - Actually assign the variable returned from PopLocalFrame
>>  - Merge test files into one
>>  - Adress review feedback from dholmes
>>  - Merge branch 'master' into JDK-8258836-check-jni-mbeanserver
>>  - 8258836: JNI local refs exceed capacity getDiagnosticCommandInfo
>
> The test looks good. You might want to say `/reviewers 2` to keep bots from assuming this approval is enough.

> I wonder if you actually have to call PopLocalFrame before returning. I couldn't find anything in the spec that indicates you need too. But there's no indication that you don't either.

@plummercj OK, I did go over the code once more and changed the `EXCEPTION_CHECK_AND_FREE` macro slightly so that it now also handles popping of local frames. If the popping isn't needed, so be it, but I guess this is now a more correct version. Leaves pushing/popping of local frames consistent no matter if we short-return or not. Do you think that's OK now?

> test/jdk/com/sun/management/DiagnosticCommandMBean/DcmdMBeanTestCheckJni.java line 60:
> 
>> 58: class DcmdMBeanRunner {
>> 59: 
>> 60:     private static String HOTSPOT_DIAGNOSTIC_MXBEAN_NAME =
> 
> Excess newline and the filed could be `static final`. Actually, you might even go and merge both classes, and then pass a fake java arg in `main` to disambiguate the "driver" and "test" code, like:
> 
>     public static void main(String[] args) {
>         if (args.length == 0) {
>             driver();
>         } else {
>             runner();
>         }
>     }
> 
> Your call.

Thanks. I've made `HOTSPOT_DIAGNOSTIC_MXBEAN_NAME` final too. The line break is so that the line doesn't exceed 80 chars. Not sure if that's still something to consider, but it shouldn't really do any harm, so I've left it. I'm on the fence about `DcmdMBeanRunner` class. Seems a tad easier to read to me as is rather than the conditional code. Hope that's OK.

-------------

PR: https://git.openjdk.java.net/jdk/pull/2130