[verona.stage] RFR 8087203: Add support for PATCH field and remove unused fields of new version string

[David Holmes]

On 19/06/2015 8:56 AM, Alejandro E Murillo wrote:
>
> Hi David,
> thanks for the review, see below
>
> On 6/18/2015 1:40 AM, David Holmes wrote:
>> Hi Alejandro,
>>
>> I looked at the hotspot and JDK changes.
>>
>> On 17/06/2015 8:55 AM, Alejandro E Murillo wrote:
>>>
>>> Please review these changes:
>>>
>>> Bug:  https://bugs.openjdk.java.net/browse/JDK-8087202
>>> Webrev: http://cr.openjdk.java.net/~amurillo/9/8087202
>>
>> hotspot/make/Makefile
>>
>> + #  VERSION_PATCH Security number for version (e.g. 0)
>>
>> Comment is wrong.
> cut and paste. Will fix
>>
>> ---
>>
>> hotspot/src/share/vm/prims/jvm.cpp
>>
>> 3659   info->patch_version = 0;
>>
>> Why is this hard-wired to zero? Surely this should be a build variable.
> good catch! That should be initialized from VM_Version as well
> (was mimicking update version, but that was always zero for hotspot
> which is not the case for patch). New webrev:
> http://cr.openjdk.java.net/~amurillo/9/8087202.v2/hotspot/src/share/vm/prims/jvm.cpp.udiff.html

Looks good.

>> hotspot/src/share/vm/prims/jvm.h
>> jdk/src/java.base/share/native/include/jvm.h
>>
>> These two files should be identical, but now have different comments
>> in places.
> will add the missing comments

Ok.

>>
>> 1188     unsigned int patch_version : 8;
>> 1215     unsigned int patch_version : 8;
>>
>> In each case you removed two 8 bit fields and added one 8 bit field so
>> your bitfields no longer add up to 32 (8+8+16). But these structs seem
>> obsolete (due to no hotspot-express) even before the new version
>> changes. So I think there is still a bit of follow up work to do here.
> oh yes,  looks like I need to keep the size of the whole structure,
> to avoid any misalignment that may  impact performance.
> I thought about declaring patch to be short (16) instead,
> but I ended up adding a padding member, reserved3, size 8,
> to keep the structure size unchanged. I guess I could have changed
> reserved2 to 24, let me know if there's any advantage using either of
> those.
> New webrev:
> http://cr.openjdk.java.net/~amurillo/9/8087202.v2/hotspot/src/share/vm/prims/jvm.h.udiff.html

Looks okay, but I still think there is scope to revisit the whole 
purpose of this struct and the use of the reserved fields etc.

Adding the 8-bit padding was the right call.

>> hotspot/src/share/vm/runtime/java.hpp
>>
>> 110                   _partially_initialized(false),
>>
>> Isn't the whole partially vs fully initialized issue dead now? Else
>> why delete the other code pertaining to that?
> yeah, I should have removed the remaining uses of that.
> It's gone now.

Looks good.

>> jdk/src/java.base/share/classes/sun/misc/Version.java.template
>>
>>   * @since JDK9
>>
>> Is that the right format? I would have expected @since 9.
> I meant to double check that. I changed it

Ok. As Alan says the use of 9 versus 1.9 will need to be sorted out but 
that's a separate issue.

>>
>>  248     private static native boolean getJvmVersionInfo();
>>
>> Does that method still need to return a boolean ? See next comment
>>
>> ---
>>
>> jdk/src/java.base/share/native/libjava/Version.c
>>
>>   50     if (!JDK_InitJvmHandle()) {
>>   51         JNU_ThrowInternalError(env, "Handle for JVM not found for
>> symbol lookup");
>>   52         return JNI_FALSE;
>>   53     }
>>   54     func_p = (GetJvmVersionInfo_fp)
>> JDK_FindJvmEntry("JVM_GetVersionInfo");
>>   55     if (func_p == NULL) { --
>>   56         return JNI_FALSE;
>>   57     }
>>
>> this seems dead code now and so the method can't fail or throw an
>> exception.
> I thought about changing that, but I prefer if it's done
> as a separate change for now. will file a follow up bug,
> Are you ok with that?

Follow up RFE is fine.

> New webrev here (I tested these with JPRT, testsets hotspot and pit):
>
> http://cr.openjdk.java.net/~amurillo/9/8087202.v2/

No further comments from me.

Thanks,
David

> Thanks
> Alejandro
>
>>
>> Thanks,
>> David
>> -----
>>
>>> These are intended to:
>>>
>>> (1)  Add support for the patch field of the new version string format
>>> (2) Remove unused fields remaining from the old version string format
>>> (3) Patch some jaxp initialization code to be able to handle the new
>>> version string.
>>>        this will be pushed under a different bug number, but is required
>>>        to be able to run and pass all the tests associated with
>>> "-testset hotspot"
>>>        (for JFR tests the VM can't be started without that).
>>>       These will be pushed under this sub-task:
>>> https://bugs.openjdk.java.net/browse/JDK-8098588
>>>
>>> (4) Additional notes:
>>> (a) Some pieces of code, only necessary for 1.5 or older,  were removed
>>> (b) update_version and special_update_version were removed
>>> (c) The code to parse the version string in
>>> jdk/test/sun/misc/Version/Version.java
>>>        will probably change when the Version.java class is available.
>>> (d) As with the changes for 8085822, this is all being pushed to [1] and
>>> then later to jdk9/dev
>>> (e) These changes should address most, if not all, of the issues raised
>>> during
>>> the code review for JDK-8085822 (see also
>>> https://bugs.openjdk.java.net/browse/JDK-8087203)
>>> (f) All  builds and tests pass for "-testset hotspot".
>>>
>>> [1] http://hg.openjdk.java.net/verona/stage
>>>
>>> Thanks
>>>
>