8175085: [REDO] G1 Needs pre barrier on dereference of weak JNI handles
Stuart Monteith
stuart.monteith at linaro.org
Fri Feb 24 16:31:19 UTC 2017
Hi,
I can build and test the patch on AArch64. I can post the results here.
BR,
Stuart
On 24 February 2017 at 09:55, 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