RFR: JDK-8085822 JEP 223: New Version-String Scheme (initial integration)

Wed Jun 10 17:21:59 UTC 2015

On 6/10/2015 6:13 AM, Magnus Ihse Bursie wrote:
> On 2015-06-10 11:58, David Holmes wrote:
>> Hi Magnus,
>>
>> Generally looks good - a few comments/queries below.
>
> In general, I believe most issues you found are valid. :-) However, as 
> I said before in this thread, I'd like to see them resolved in the 
> needed follow-up patches. The source code changes in Hotspot and JDK 
> are inadequate to fully implement JEP-223, so these areas will need to 
> be rewritten anyhow. Are you okay with resolving these issues later?
>
>>
>> On 9/06/2015 11:52 PM, Magnus Ihse Bursie wrote:
>>> Here's an updated webrev, which fixes the typos that were pointed 
>>> out by
>>> reviewers:
>>> http://cr.openjdk.java.net/~ihse/JDK-8085822-JEP-223-initial-patch/webrev.02/ 
>>>
>>
>> common/autoconf/version-numbers
>>
>> Shouldn't there be a DEFAULT_VERSION_XXX for each of the XXX parts of 
>> the version info? (Even if all zeroes presently.)
> So that's a tricky one. :-& The question here is, I think, does it 
> make sense to update version-numbers when we do a security (9.0.1) or 
> minor (9.1.0) release? Looking historically, the version-numbers file 
> have not been changed for the 8u family. The only change was going 
> from 8 to 9 when creating the new jdk9 forest. That was what I based 
> my decition to only have the major number in the file. (The rest of 
> the version string is set by configure flags when building, in Oracle 
> case by the RE team.)
>
> If it makes sense to put the minor/security/patch numbers here, I 
> won't oppose it, but I guess we have until the first such release to 
> figure out what's best, and that likely includes discussion with RE 
> and possibly other consumers in the community (RedHat etc).
>
>>
>> ---
>>
>> Looking at hotspot changes I can't convince myself that the new 
>> streamlined "version" variables will capture things like "fastdebug". 
>> Will that filter through via configure variables?
>
> The "fastdebug" label has been handled as a less valued token in the 
> JEP-223 process. Right now there's no mention of it at all in the 
> version string proposal in the JEP. As I understand it, Iris is about 
> to propose an amendment which will just make fastdebug be a part of 
> the OPT string. I'm not entirely happy with that myself, but that's 
> the way it's decided. So wherever you can see the OPT string, you'll 
> see the debug level tag.
>
> Currently, however, that's not how it is implemented in this patch. 
> Since no decision was made on this (and it's still not formally 
> decided), I had to pick a route to go forward. The current patch will 
> instead put the "fastdebug" label as part of the PRE string (that's 
> the reason for the pre-base and pre-debuglevel arguments to 
> configure). If the planned changes goes into the JEP, this'll change 
> to make the debuglevel tag a part of the OPT string instead.
>
>> ---
>>
>> make/*/makefiles/vm.make
>>
>> Shouldn't the -DVERSION_XX=$(VERSION_XX) definitions be taken from 
>> the VERSION_CFLAGS in spec.gmk? (Or are you still allowing for a 
>> stand-alone hotspot build?)
> The hotspot JEP-223 work initially made by Alejandro kept support for 
> stand-alone hotspot builds. I made additional changes on top of that, 
> which still tried to cater for stand-alone builds. At this very moment 
> I'm not sure if stand-alone hotspot builds are supposed to work or 
> not, but I've tried to not change the situation with this patch.
>
> There are two future changes coming down the pipe: One is the 
> additional JEP-223 work needed for Hotspot. I know Alejandro had plans 
> for redesigning the API between Hotspot and the JDK, so no JDK version 
> strings should be compiled into the JVM, but all of it extracted 
> during an API in runtime. That would make most (all?) of the makefile 
> changes in hotspot irrelevant. However, that implementation is not 
> even started, so it's needed for the time being.
There's already an API used by Hotspot to get  JDK version values.
I'm investigating using that and extend it if required.
yes, there is a lot stuff in the hotspot code that should be removed
(mostly in the makefiles)
and I'm not against making those changes now, I just think they are
somewhat out of the scope of this project. And  should be done
as individual RFEs or as part of the upcoming hotspot makefile refactoring.

Alejandro
>
> The second change is the build-infra hotspot makefile rewrite. When 
> that happens, stand-alone builds will definitely disappear, and if 
> Hotspot still needs make support for version strings, then the logical 
> choice is to use VERSION_CFLAGS.
>
> Ok?
>
>>
>> ---
>>
>> hotspot/src/share/vm/prims/jvm.h
>> jdk/src/java.base/share/native/include/jvm.h
>>
>> I think this comment is way out of date:
>>
>>  /* Build number is available only for RE build (i.e. 
>> JDK_BUILD_NUMBER is set to bNN)
>>    * It will be zero for internal builds.
>>   */
>>
>> and can be completely removed. Even if still true, mention of an "RE 
>> build" has no place in OpenJDK sources.
> Yep, agree. Follow-up patch, right?
>
>>
>> ---
>>
>> hotspot/src/share/vm/runtime/java.cpp
>>
>> I think a lot of the modified code is obsolete post Hotspot Express - 
>> which makes it hard to determine if the changes make sense.
> Yep, agree. Follow-up patch, right?
>>
>> ---
>>
>> hotspot/src/share/vm/runtime/vmStructs.cpp
>>
>> Please fix the alignment (3 extra spaces on modified line):
>>
>> 1239   static_field(Abstract_VM_Version, _vm_minor_version, 
>> int)                                   \
>> 1240   static_field(Abstract_VM_Version, _vm_security_version, 
>> int)                  \
>
> I was about to say "follow-up patch", but ugly indentation really 
> sucks so I can fix this. Ok if I skip redoing the review for that?
>
>>
>> ---
>>
>> hotspot/test/runtime/6981737/Test6981737.java
>>
>> This test is really only valid for the new version scheme now, so it 
>> should probably be rewritten - and in that case it really isn't 
>> testing 6981737 but should be replaced by a new test for the new 
>> version format.
> Yep, agree. Follow-up patch, right?
>>
>> ---
>>
>> jdk/src/java.base/share/classes/sun/misc/Version.java.template
>>
>> This comment is nonsensical:
>>
>>       /**
>> !      * Returns the security version of the running JVM if it's 1.6 
>> or newer
>>        * or any RE VM build. It will return 0 if it's an internal 1.5 or
>>        * 1.4.x build.
>>        * @since 1.6
>>        */
>>
>> as security version does not exist pre 9. Normally you should be 
>> adding a new method and deprecating the old one. The new one is 
>> @since 9.
> Yep, agree. Follow-up patch, right?
>>
>>      /**
>> !      * Returns the security version of the running JDK.
>>        * @since 1.6
>>        */
>>
>> Ditto: @since 9 (but again old should be deprecated and new method 
>> added)
>>
>> 253     /**
>>  254      * Returns the build number of the running JDK if it's a RE 
>> build
>>  255      * It will return 0 if it's an internal build.
>>
>> As with jvm.h this seems obsolete commentary these days - not only RE 
>> builds define a build number.
> Yep, agree. Follow-up patch, right?
>
> /Magnus
>
>>
>> Thanks,
>> David
>
-- 
Alejandro