8175085: [REDO] G1 Needs pre barrier on dereference of weak JNI handles
Mikael Gerdin
mikael.gerdin at oracle.com
Fri Feb 24 09:55:32 UTC 2017
Hi Dan,
Thanks for looking at this review!
On 2017-02-23 17:18, Daniel D. Daugherty wrote:
> On 2/23/17 6:29 AM, Mikael Gerdin wrote:
>> Hi Kim,
>>
>> Thanks for the prompt review!
>>
>> On 2017-02-22 20:40, Kim Barrett wrote:
>>>> On Feb 22, 2017, at 11:07 AM, Mikael Gerdin
>>>> <mikael.gerdin at oracle.com> wrote:
>>>>
>>>> Hi all,
>>>> Please review this revised change to jweak to support G1.
>>>>
>>>> I've copied Kim's original description of the fix below for reference:
>>>>
>>>>>
>>>>> The problem being addressed is that, when using G1, dereferencing a
>>>>> jweak must ensure the obtained referent is ensured live, just as for
>>>>> other weak reference types. This has always been a requirement, and
>>>>> failure to do so appears to be a since-day1 G1 bug.
>>>>>
>>>>> There are two categories of places that need to address this issue:
>>>>> JNIHandles::resolve and friends, and returning a jobject from native
>>>>> code to Java. In both of these places the jobject whose contained oop
>>>>> is being extracted might really be a jweak. jweak is
>>>>> representationally indistinguishable from jobject; jweak is just a
>>>>> typedef for jobject. The documentation says a jweak can be used
>>>>> anywhere a jobject is permitted, making that type equivalence
>>>>> necessary for a C API. (A C++ API might be able to have distinct
>>>>> types via inheritance, though would still need a method for
>>>>> distinguishing them at runtime. But since JNI is a C API...).
>>>>>
>>>>> The basic approach being taken is to "tag" jweaks by setting the low
>>>>> order bit (recall that jweak == jobject == _jobject*), in order to
>>>>> distinguish them from non-jweak jobjects. This tagging is only being
>>>>> done when the selected GC needs the distinction, e.g. G1. This gives
>>>>> us a representational difference between jobject and jweak on which to
>>>>> base the different behavior, hiding that distinction behind the
>>>>> existing API.
>>>>>
>>>>> JNIHandles::resolve and friends are changed to unconditionally strip
>>>>> off any potential weak tag when translating from jobject to to oop*.
>>>>> That's cheaper than testing for the conditional use of tagging and
>>>>> then stripping. Also stripped when deleting JNI handles.
>>>>>
>>>>> TemplateInterpreterGenerator::generate_native_entry and
>>>>> SharedRuntime::generate_native_wrapper are changed to conditionally
>>>>> emit code to apply the G1 pre-barrier to the value obtained from a
>>>>> jobject result, when the result is tagged as a jweak, which only
>>>>> occurs when using G1.
>>>>>
>>>>> For arm32/arm64, this required moving the g1_write_barrier_pre
>>>>> definitions from InterpreterMacroAssembler to MacroAssembler. Also
>>>>> moved g1_write_barrier_post, for consistency.
>>>>>
>>>>
>>>> In addition to Kim's final version of the change (which was backed
>>>> out) this updated version adds code to mask out the weak tag bit in
>>>> the fast JNI field getters on all platforms where fast JNI getters
>>>> are used.
>>>>
>>>> […]
>>>>
>>>> Webrevs:
>>>> Kim's initial change:
>>>> http://cr.openjdk.java.net/~mgerdin/8175085/webrev.kim/webrev/
>>>> My additions:
>>>> http://cr.openjdk.java.net/~mgerdin/8175085/webrev.mg/webrev/
>>>> Full webrev:
>>>> http://cr.openjdk.java.net/~mgerdin/8175085/webrev.full/
>>>>
>>>> Bugs:
>>>> https://bugs.openjdk.java.net/browse/JDK-8175085
>>>> https://bugs.openjdk.java.net/browse/JDK-8166188
>>>>
>>>> Thanks
>>>> /Mikael
>>>
>>> Mikael - Thanks for taking this up for me. Just a few minor comments.
>>>
>>> ------------------------------------------------------------------------------
>>>
>>> The usual copyright reminder.
>>
>> Updated copyrights.
>>
>>>
>>> ------------------------------------------------------------------------------
>>>
>>> src/cpu/arm/vm/jniFastGetField_arm.cpp
>>> 122 #ifndef AARCH64
>>> 123 __ bic(R1, R1, JNIHandles::weak_tag_mask);
>>> 124 #else
>>> 125 __ andr(R1, R1, ~JNIHandles::weak_tag_mask);
>>> 126 #endif
>>>
>>> Usual style when selecting between AARCH64 and not seems to be to put
>>> the AARCH64 code first, e.g.
>>>
>>> #ifdef AARCH64
>>> ...
>>> #else
>>> ...
>>> #endif
>>
>> Fixed.
>>
>>>
>>> ------------------------------------------------------------------------------
>>>
>>> src/cpu/x86/vm/jniFastGetField_x86_32.cpp
>>> 90 assert(inverted_jweak_mask == -2, "Otherwise check this code");
>>>
>>> Use STATIC_ASSERT rather than assert.
>>>
>>> ------------------------------------------------------------------------------
>>>
>>> src/cpu/x86/vm/jniFastGetField_x86_32.cpp
>>> 89 const intptr_t inverted_jweak_mask =
>>> ~static_cast<intptr_t>(JNIHandles::weak_tag_mask);
>>> 90 assert(inverted_jweak_mask == -2, "Otherwise check this code");
>>> 91 __ andl(rdx, inverted_jweak_mask); // mask is subject to
>>> sign-extension
>>>
>>>
>>> With three occurrences of this snippet in this file and two more
>>> similar ones in the 64-bit file, a macroAssembler helper seems called
>>> for here.
>>>
>>> ------------------------------------------------------------------------------
>>>
>>> src/cpu/x86/vm/jniFastGetField_x86_64.cpp
>>> 84 const intptr_t inverted_jweak_mask =
>>> ~static_cast<intptr_t>(JNIHandles::weak_tag_mask);
>>> 85 const int32_t truncated_mask =
>>> static_cast<int32_t>(inverted_jweak_mask);
>>>
>>> Why the two-step conversion here? Why not just
>>>
>>> const int32_t truncated_mask =
>>> static_cast<int32_t>(JNIHandles::weak_tag_mask);
>>
>> I had changed this a bunch of times since I was a bit unsure about how
>> to best do this but when I decided to do the bitwise invert after the
>> cast to signed I forgot to clean up the casts.
>>
>>>
>>> That would also make the 32 and 64-bit code more similar in the
>>> suggested macroAssembler helper.
>>
>> Indeed, I even found MacroAssembler::andptr which makes this a nice
>> and small helper.
>>
>>>
>>> ------------------------------------------------------------------------------
>>>
>>> test/runtime/jni/CallWithJNIWeak/libCallWithJNIWeak.c
>>> 51 #define CHECK(variable, expected) \
>>> 52 do { if ((variable) != (expected)) { \
>>> 53 (*env)->ThrowNew(env, exception, #variable" != " #expected); \
>>> 54 return; \
>>> 55 } \
>>> 56 } while(0);
>>>
>>> The formatting of the code in the macro body is strange and confusing.
>>
>> Cleaned up formatting.
>>
>>>
>>> Also the do-while-zero idiom that I'm used to leaves off the final
>>> semicolon in the macro. Instead of the uses are expected to be
>>> terminated with semicolons, as you've done.
>>
>> Oh, oops.
>>
>> I also took the liberty to add STATIC_ASSERTS on 64 bit arm since the
>> encoding of the immediate in the "andr" instruction is a bit "funny".
>> The assembler code only verifies that immediate encoding works in
>> debug builds but in debug builds the fast JNI getters aren't generated.
>>
>> New webrevs:
>> Incremental:
>> http://cr.openjdk.java.net/~mgerdin/8175085/webrev.incremental/webrev/
>> Full:
>> http://cr.openjdk.java.net/~mgerdin/8175085/webrev.full2/webrev/
>
> I only took a look at these files from the full webrev. These are the
> files touch in the two incremental webrevs:
>
> src/cpu/aarch64/vm/jniFastGetField_aarch64.cpp
> No comments.
>
> src/cpu/arm/vm/jniFastGetField_arm.cpp
> No comments.
>
> src/cpu/sparc/vm/jniFastGetField_sparc.cpp
> No comments.
>
> src/cpu/x86/vm/jniFastGetField_x86_32.cpp
> No comments.
>
> src/cpu/x86/vm/jniFastGetField_x86_64.cpp
> No comments.
>
> src/cpu/x86/vm/macroAssembler_x86.cpp
> I like the new helper!
>
> src/cpu/x86/vm/macroAssembler_x86.hpp
> No comments.
>
> test/runtime/jni/CallWithJNIWeak/CallWithJNIWeak.java
> L50 - any reason for the blank line?
No reason whatsoever, I'll remove it.
>
> test/runtime/jni/CallWithJNIWeak/libCallWithJNIWeak.c
> L32 : Java_CallWithJNIWeak_testJNIFieldAccessors (JNIEnv *env,
> jclass clazz, jobject this) {
> L102: Java_CallWithJNIWeak_runTests (JNIEnv *env, jclass clazz,
> jobject this) {
> L137: Java_CallWithJNIWeak_weakReceiverTest0 (JNIEnv *env, jobject
> obj) {
> Please delete the space before the first '('.
Oh, oops. I'm going to blame javah for that :)
>
> You may want to add a comment between the two halves of
> the test for the literal values that you're setting in
> the .java file and testing for in the .c file.
Added short comments in the .java and .c files.
>
> In your (Mikael G) original invite:
>
>> Testing:
>> Standard JPRT and tier2-3 HotSpot tests.
>
> Does tier2 and/or tier3 cover the tests that failed in Mach5?
Somehow I forgot to mention that I also ran jdk_tier3 and the tests that
failed in Mach5 did pass on all platforms.
>
>
>> I've modified the CallWithJNIWeak test to call all the primitive
>> getters and some other interesting cases I came up with.
>> I've also run the CallWithJNIWeak test on all platforms on both
>> product and fastdebug builds (since fast JNI getters are implicitly
>> disabled in debug builds due to VerifyJNIFields being enabled by
>> default in debug builds.
>
> Great coverage with the new tests!
>
>
>> I've not built or tested the aarch64 port but I think it's correct
>> and I hope someone can test it for me.
>
> Have you heard back from anyone on aarch64? I haven't
> seen any e-mail on this OpenJDK thread...
I've cc:ed Andrew in this thread to see if I can catch his attention.
New webrevs at:
http://cr.openjdk.java.net/~mgerdin/8175085/webrev.full3/webrev/
http://cr.openjdk.java.net/~mgerdin/8175085/webrev.incremental2/webrev/
Thanks
/Mikael
>
> Thumbs up!
>
> Dan
>
>
>>
>> Thanks
>> /Mikael
>>
>>>
>>> ------------------------------------------------------------------------------
>>>
>>>
>
More information about the hotspot-dev
mailing list