RFR 8143628: Fork sun.misc.Unsafe and jdk.internal.misc.Unsafe native method tables

[Coleen Phillimore]

On 12/3/15 4:51 AM, Paul Sandoz wrote:
>> On 3 Dec 2015, at 03:58, Coleen Phillimore <coleen.phillimore at oracle.com> wrote:
>>
>>
>> I finally had a chance to look at this.  Why are there pages of commit messages in these webrevs?
>>
> Not sure, it seems to be a bug with webrev (and may be a consequence of the interaction with patch queues, but i have not had time to dig into why this occurs).

I think this is a bug with the interaction with your patch queues. 
Before you send stuff out for review (at least to the hotspot lists), 
can you export the patch into a clean repository so that we don't have 
to find the changes in pages of stuff?
>
>
>> http://cr.openjdk.java.net/~psandoz/jdk9/JDK-8143628-unsafe-native-hotspot/webrev/src/share/vm/prims/unsafe.cpp.udiff.html
>>
>> +static JNINativeMethod sun_misc_Unsafe_methods[] = {
>>
>> This looks like a copy of the list of methods that we already have.
>>
> It's currently a subset of jdk_internal_misc_Unsafe_methods, notice that the entries for the following are not present in sun_misc_Unsafe_methods:
>
>    private native boolean unalignedAccess0();
>    private native boolean isBigEndian0();
>
> The internal one will further diverge later on as we include additional access methods, and may likely becoming intersecting sets. Hence i deliberately kept them separate.
>
>
>> +static JNINativeMethod jdk_internal_misc_Unsafe_methods[] = {
>>
>> Can you just have:
>>
>> +static JNINativeMethod sun_misc_Unsafe_methods[] = methods; +static JNINativeMethod jdk_internal_misc_Unsafe_methods[] = methods; ???
>>
>> http://cr.openjdk.java.net/~psandoz/jdk9/JDK-8143628-unsafe-native-hotspot/webrev/test/runtime/Unsafe/JdkInternalMiscUnsafeAccessTestBoolean.java.html
>>
>> I don't see the benefit of testng here.
> The benefits are:
>
>    1) Each test is clearly identified
>    2) Each test runs regardless of the failure of any other test
>    3) Each test result is reported
>
>
>> You probably already have Assert in our test library.
>>
>> http://cr.openjdk.java.net/~psandoz/jdk9/JDK-8143628-unsafe-native-hotspot/webrev/test/runtime/Unsafe/generate-unsafe-access-tests.sh.html
>>
>> Is this script and template how these tests were generated?
> Yes, it’s the same pattern as used in nio tests. If we evolve the tests we will edit the template and run the generation script rather than potentially editing 18 files.

That seems ok, as long as running the script isn't part of the testing.
>
>> It doesn't seem like it needs to be checked in (and is missing copyright).
>>
> Ah, thanks, i will add the copyright.
>
>
>> Maybe since the tests are stressing the compilation characteristics of the unsafe API, these tests belong in the test/compiler/unsafe directory.  Maybe the compiler group doesn't mind slogging through testng to find bugs.
>>
> But how much of an actual slog is it? (see my reply to you on hotspot-dev)
>
> If i cannot persuade you and others in the runtime team (my powers of persuasion are failing and it is looking unlikely, but i am still trying :-) ), i will move them to the idk test area.

As I said in my last email, the test should be in the hotspot test 
repositories because it seems like compiler changes could break them and 
we'd want to run the tests with each putback.   See if the compiler 
group can tolerate an additional harness.

Coleen

> Thanks,
> Paul.