RFR: 8193222: EnsureLocalCapacity() should maintain capacity requests through multiple calls

classic Classic list List threaded Threaded
6 messages Options
Reply | Threaded
Open this post in threaded view
|

RFR: 8193222: EnsureLocalCapacity() should maintain capacity requests through multiple calls

David Holmes
bug: https://bugs.openjdk.java.net/browse/JDK-8193222
webrev: http://cr.openjdk.java.net/~dholmes/8193222/webrev/

In product mode, for hotspot, EnsureLocalCapacity is a no-op as there is
no artificial limit applied for local refs.

With -Xcheck:jni the checked mode was augmented to watch for "excessive"
use of local refs and to produce a warning if that happened e.g.:

  WARNING: JNI local refs: 41, exceeds capacity: 40

That was added under JDK-8043224.

The problem is that the code always modifies the planned capacity (that
expected before warnings should be used) without regard for the fact the
existing capacity may be higher than that requested. As a result if you
have a call chain like:

void foo() {  // C code
   env->EnsureLocalCapacity(60); // needs lots of local refs
   ...
   JNU_GetPlatformsString(...)
       env->EnsureLocalCapacity(5); // lower than 60!
    ...
    // create 60 local refs
}

upon return the warning will be issued because the number of local refs
exceeds the most recent call to EnsureLocalCapacity.

A simple fix is for EnsureLocalCapacity to only raise the planned
capacity, not lower it. That fits with the notion of "ensuring" there is
sufficient space - the function is not SetLocalcapacity. It also fits
with the way PushLocalFrame(capacity) increases the planned capacity by
"capacity" but PopLocalFrame does not reduce it again.

New test added.

Tested through JPRT.

Thanks,
David
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8193222: EnsureLocalCapacity() should maintain capacity requests through multiple calls

David Holmes
Ping! Very simple, and trying to get in before cutoff.

Thanks,
David

On 10/12/2017 3:39 PM, David Holmes wrote:

> bug: https://bugs.openjdk.java.net/browse/JDK-8193222
> webrev: http://cr.openjdk.java.net/~dholmes/8193222/webrev/
>
> In product mode, for hotspot, EnsureLocalCapacity is a no-op as there is
> no artificial limit applied for local refs.
>
> With -Xcheck:jni the checked mode was augmented to watch for "excessive"
> use of local refs and to produce a warning if that happened e.g.:
>
>   WARNING: JNI local refs: 41, exceeds capacity: 40
>
> That was added under JDK-8043224.
>
> The problem is that the code always modifies the planned capacity (that
> expected before warnings should be used) without regard for the fact the
> existing capacity may be higher than that requested. As a result if you
> have a call chain like:
>
> void foo() {  // C code
>    env->EnsureLocalCapacity(60); // needs lots of local refs
>    ...
>    JNU_GetPlatformsString(...)
>        env->EnsureLocalCapacity(5); // lower than 60!
>     ...
>     // create 60 local refs
> }
>
> upon return the warning will be issued because the number of local refs
> exceeds the most recent call to EnsureLocalCapacity.
>
> A simple fix is for EnsureLocalCapacity to only raise the planned
> capacity, not lower it. That fits with the notion of "ensuring" there is
> sufficient space - the function is not SetLocalcapacity. It also fits
> with the way PushLocalFrame(capacity) increases the planned capacity by
> "capacity" but PopLocalFrame does not reduce it again.
>
> New test added.
>
> Tested through JPRT.
>
> Thanks,
> David
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8193222: EnsureLocalCapacity() should maintain capacity requests through multiple calls

coleen.phillimore

This looks good to me.  I've reviewed both the code and the test.

Thanks,
Coleen

On 12/11/17 5:18 PM, David Holmes wrote:

> Ping! Very simple, and trying to get in before cutoff.
>
> Thanks,
> David
>
> On 10/12/2017 3:39 PM, David Holmes wrote:
>> bug: https://bugs.openjdk.java.net/browse/JDK-8193222
>> webrev: http://cr.openjdk.java.net/~dholmes/8193222/webrev/
>>
>> In product mode, for hotspot, EnsureLocalCapacity is a no-op as there
>> is no artificial limit applied for local refs.
>>
>> With -Xcheck:jni the checked mode was augmented to watch for
>> "excessive" use of local refs and to produce a warning if that
>> happened e.g.:
>>
>>   WARNING: JNI local refs: 41, exceeds capacity: 40
>>
>> That was added under JDK-8043224.
>>
>> The problem is that the code always modifies the planned capacity
>> (that expected before warnings should be used) without regard for the
>> fact the existing capacity may be higher than that requested. As a
>> result if you have a call chain like:
>>
>> void foo() {  // C code
>>    env->EnsureLocalCapacity(60); // needs lots of local refs
>>    ...
>>    JNU_GetPlatformsString(...)
>>        env->EnsureLocalCapacity(5); // lower than 60!
>>     ...
>>     // create 60 local refs
>> }
>>
>> upon return the warning will be issued because the number of local
>> refs exceeds the most recent call to EnsureLocalCapacity.
>>
>> A simple fix is for EnsureLocalCapacity to only raise the planned
>> capacity, not lower it. That fits with the notion of "ensuring" there
>> is sufficient space - the function is not SetLocalcapacity. It also
>> fits with the way PushLocalFrame(capacity) increases the planned
>> capacity by "capacity" but PopLocalFrame does not reduce it again.
>>
>> New test added.
>>
>> Tested through JPRT.
>>
>> Thanks,
>> David

Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8193222: EnsureLocalCapacity() should maintain capacity requests through multiple calls

Daniel D. Daugherty
In reply to this post by David Holmes
On 12/10/17 12:39 AM, David Holmes wrote:
> bug: https://bugs.openjdk.java.net/browse/JDK-8193222
> webrev: http://cr.openjdk.java.net/~dholmes/8193222/webrev/

make/test/JtregNativeHotspot.gmk
     No comments.

src/hotspot/share/prims/jniCheck.cpp
     No comments.

test/hotspot/jtreg/runtime/jni/checked/TestCheckedEnsureLocalCapacity.java
     L42:     // If copies > capacity + warning-threshold then we still
get a warning
         Nit: Needs a period at the end.

     L75:         //Warning
         Nit: Needs a space before 'W'.

test/hotspot/jtreg/runtime/jni/checked/libTestCheckedEnsureLocalCapacity.c
     No comment.

Thumbs up! Your choice on whether to fix the nits.
Don't need to see another webrev even if you do...

Dan


>
> In product mode, for hotspot, EnsureLocalCapacity is a no-op as there
> is no artificial limit applied for local refs.
>
> With -Xcheck:jni the checked mode was augmented to watch for
> "excessive" use of local refs and to produce a warning if that
> happened e.g.:
>
>  WARNING: JNI local refs: 41, exceeds capacity: 40
>
> That was added under JDK-8043224.
>
> The problem is that the code always modifies the planned capacity
> (that expected before warnings should be used) without regard for the
> fact the existing capacity may be higher than that requested. As a
> result if you have a call chain like:
>
> void foo() {  // C code
>   env->EnsureLocalCapacity(60); // needs lots of local refs
>   ...
>   JNU_GetPlatformsString(...)
>       env->EnsureLocalCapacity(5); // lower than 60!
>    ...
>    // create 60 local refs
> }
>
> upon return the warning will be issued because the number of local
> refs exceeds the most recent call to EnsureLocalCapacity.
>
> A simple fix is for EnsureLocalCapacity to only raise the planned
> capacity, not lower it. That fits with the notion of "ensuring" there
> is sufficient space - the function is not SetLocalcapacity. It also
> fits with the way PushLocalFrame(capacity) increases the planned
> capacity by "capacity" but PopLocalFrame does not reduce it again.
>
> New test added.
>
> Tested through JPRT.
>
> Thanks,
> David
>

Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8193222: EnsureLocalCapacity() should maintain capacity requests through multiple calls

David Holmes
In reply to this post by coleen.phillimore
Thanks Coleen!

David

On 12/12/2017 8:53 AM, [hidden email] wrote:

>
> This looks good to me.  I've reviewed both the code and the test.
>
> Thanks,
> Coleen
>
> On 12/11/17 5:18 PM, David Holmes wrote:
>> Ping! Very simple, and trying to get in before cutoff.
>>
>> Thanks,
>> David
>>
>> On 10/12/2017 3:39 PM, David Holmes wrote:
>>> bug: https://bugs.openjdk.java.net/browse/JDK-8193222
>>> webrev: http://cr.openjdk.java.net/~dholmes/8193222/webrev/
>>>
>>> In product mode, for hotspot, EnsureLocalCapacity is a no-op as there
>>> is no artificial limit applied for local refs.
>>>
>>> With -Xcheck:jni the checked mode was augmented to watch for
>>> "excessive" use of local refs and to produce a warning if that
>>> happened e.g.:
>>>
>>>   WARNING: JNI local refs: 41, exceeds capacity: 40
>>>
>>> That was added under JDK-8043224.
>>>
>>> The problem is that the code always modifies the planned capacity
>>> (that expected before warnings should be used) without regard for the
>>> fact the existing capacity may be higher than that requested. As a
>>> result if you have a call chain like:
>>>
>>> void foo() {  // C code
>>>    env->EnsureLocalCapacity(60); // needs lots of local refs
>>>    ...
>>>    JNU_GetPlatformsString(...)
>>>        env->EnsureLocalCapacity(5); // lower than 60!
>>>     ...
>>>     // create 60 local refs
>>> }
>>>
>>> upon return the warning will be issued because the number of local
>>> refs exceeds the most recent call to EnsureLocalCapacity.
>>>
>>> A simple fix is for EnsureLocalCapacity to only raise the planned
>>> capacity, not lower it. That fits with the notion of "ensuring" there
>>> is sufficient space - the function is not SetLocalcapacity. It also
>>> fits with the way PushLocalFrame(capacity) increases the planned
>>> capacity by "capacity" but PopLocalFrame does not reduce it again.
>>>
>>> New test added.
>>>
>>> Tested through JPRT.
>>>
>>> Thanks,
>>> David
>
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8193222: EnsureLocalCapacity() should maintain capacity requests through multiple calls

David Holmes
In reply to this post by Daniel D. Daugherty
Thanks Dan! Nits will be fixed.

David

On 12/12/2017 11:00 AM, Daniel D. Daugherty wrote:

> On 12/10/17 12:39 AM, David Holmes wrote:
>> bug: https://bugs.openjdk.java.net/browse/JDK-8193222
>> webrev: http://cr.openjdk.java.net/~dholmes/8193222/webrev/
>
> make/test/JtregNativeHotspot.gmk
>      No comments.
>
> src/hotspot/share/prims/jniCheck.cpp
>      No comments.
>
> test/hotspot/jtreg/runtime/jni/checked/TestCheckedEnsureLocalCapacity.java
>      L42:     // If copies > capacity + warning-threshold then we still
> get a warning
>          Nit: Needs a period at the end.
>
>      L75:         //Warning
>          Nit: Needs a space before 'W'.
>
> test/hotspot/jtreg/runtime/jni/checked/libTestCheckedEnsureLocalCapacity.c
>      No comment.
>
> Thumbs up! Your choice on whether to fix the nits.
> Don't need to see another webrev even if you do...
>
> Dan
>
>
>>
>> In product mode, for hotspot, EnsureLocalCapacity is a no-op as there
>> is no artificial limit applied for local refs.
>>
>> With -Xcheck:jni the checked mode was augmented to watch for
>> "excessive" use of local refs and to produce a warning if that
>> happened e.g.:
>>
>>  WARNING: JNI local refs: 41, exceeds capacity: 40
>>
>> That was added under JDK-8043224.
>>
>> The problem is that the code always modifies the planned capacity
>> (that expected before warnings should be used) without regard for the
>> fact the existing capacity may be higher than that requested. As a
>> result if you have a call chain like:
>>
>> void foo() {  // C code
>>   env->EnsureLocalCapacity(60); // needs lots of local refs
>>   ...
>>   JNU_GetPlatformsString(...)
>>       env->EnsureLocalCapacity(5); // lower than 60!
>>    ...
>>    // create 60 local refs
>> }
>>
>> upon return the warning will be issued because the number of local
>> refs exceeds the most recent call to EnsureLocalCapacity.
>>
>> A simple fix is for EnsureLocalCapacity to only raise the planned
>> capacity, not lower it. That fits with the notion of "ensuring" there
>> is sufficient space - the function is not SetLocalcapacity. It also
>> fits with the way PushLocalFrame(capacity) increases the planned
>> capacity by "capacity" but PopLocalFrame does not reduce it again.
>>
>> New test added.
>>
>> Tested through JPRT.
>>
>> Thanks,
>> David
>>
>