8175085: [REDO] G1 Needs pre barrier on dereference of weak JNI handles
Mikael Gerdin
mikael.gerdin at oracle.com
Fri Feb 24 11:54:29 UTC 2017
Hi Volker,
On 2017-02-24 12:37, Volker Simonis wrote:
> Hi Mikael,
>
> I had a little problems to apply the patch from your webrev because
> the file actually contains two patches [1] but after fixing that
> manually, I could still build and run the test on ppc64 and s390x.
I seem to recall that that's a known limitation in webrev.
Thanks for verifying the fix on your side.
/Mikael
>
> Thanks,
> Volker
>
> [1] http://cr.openjdk.java.net/~mgerdin/8175085/webrev.full/webrev/hotspot.changeset
>
> On Fri, Feb 24, 2017 at 10:55 AM, Mikael Gerdin
> <mikael.gerdin at oracle.com> wrote:
>> 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