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
