RFR: JDK-8085822 JEP 223: New Version-String Scheme (initial integration)
Magnus Ihse Bursie
magnus.ihse.bursie at oracle.com
Thu Jun 11 12:14:59 UTC 2015
On 2015-06-11 06:52, David Holmes wrote:
> Hi Magnus,
>
> On 10/06/2015 10:13 PM, 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?
>
> Given this is going to a staging repo I'm okay with deferring things -
> I just have a concern whether such things may be overlooked given that
> the integration with the staging repo will not be undergoing a final
> review.
I agree completely with your concern. I have summarized the issues that
were raised but not addressed during this review, and created JBS bugs,
one per component. I will do my best to make sure that fixing them does
not get lost from the Verona project agenda.
The three bugs are:
https://bugs.openjdk.java.net/browse/JDK-8087202
https://bugs.openjdk.java.net/browse/JDK-8087203
https://bugs.openjdk.java.net/browse/JDK-8087205
Here's the core content of them. If I have missed something, please add
it to the bug reports:
HOTSPOT:
Alan Bateman:
Will the update_version and special_update_version fields eventually be
dropped from the jvm_version_info structure?
The webrev shows a change to this comment in jvm.h:
"Third, this file contains various I/O and network operations needed
by the standard Java I/O and network APIs."
I think this comment can be removed because those JVM_* functions
were removed some time ago.
Daniel D. Daugherty:
General comment: It looks like support for the 'patch' value is not
completely
implemented through all the Makefiles. I didn't audit for this, but
it's
just my impression.
hotspot/src/share/vm/runtime/java.cpp
L661: void JDK_Version::fully_initialize(
L662: uint8_t major, uint8_t minor, uint8_t security, uint8_t
update) {
L663: // This is only called when current is less than 1.6 and
we've gotten
Since you're ripping out vestigial version support, I think this
function can go away since this is version 9 and newer. Don't
really
know for sure, but based on that comment...
hotspot/src/share/vm/runtime/vm_version.cpp
L84: void Abstract_VM_Version::initialize() {
L85: // FIXME: Initialization can probably be removed now.
I agree, but that would entail also removing the
_initialized and the uses of it... Follow on bug fix?
David Holmes:
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?)
hotspot/src/share/vm/prims/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.
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.
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.
JDK:
Alan Bateman:
Version.java.template - the comment in jvmSecurityVersion() still talks
about 1.6 and newer. Can this be replaced to just say that it returns
the security version?
Daniel D. Daugherty:
jdk/src/java.base/share/classes/sun/misc/Version.java.template
L149: * Returns the security version of the running JVM if it's 1.6
or newer
This JavaDoc update is wrong, but it might not be important
if sun.misc.Version class is going away.
David Holmes:
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.
/**
! * 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.
LANGTOOLS:
Daniel D. Daugherty:
langtools/src/java.compiler/share/classes/javax/lang/model/SourceVersion.java
old L171: case "1.9":
new L171: case "9":
Should this logic support both versions? Will dropping
"1.9" here prevent a pre-version changeset JVM from
being dropped into a JDK for triage purposes?
Granted we don't often triage 'javac' with different JVMs, but...
Jan Lahoda:
+1 on keeping both "1.9" and "9" here. javac can be used independently
on the rest of JDK to some extent, so support for running it on older
builds of JDK 9 seems reasonable to me. (I wonder if current JDK 9 javac
should be prepared for the new version scheme in advance.)
/Magnus
>
> I would prefer to see the Version.java.template doc comments
> corrected, even if the issue of whether to add and deprecate is
> deferred, but again as this is to a staging area I can let it slide
> for now.
>
> I saw the fix to hotspot/src/share/vm/runtime/vmStructs.cpp.
>
> Thanks,
> David
>
>>>
>>> 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.
>>
>> 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
>>
More information about the nashorn-dev
mailing list