RFR 7041262: VM_Version should be called instead of Abstract_VM_Version so that overriding works (original) (raw)

David Holmes david.holmes at oracle.com
Tue Oct 16 05:14:02 UTC 2018

Hi Harold,

On 13/10/2018 1:31 AM, Harold David Seigel wrote:

Hi,

Please review this fix for JDK-7041262.  The fix changes calls to AbstractVMVersion functions to instead call VMVersion functions.  See details in the bug.

There is an issue with the vm_info_string() changes.

On ARM32 we have an "overriding" version of VM_Version::VM_info_string() which will now get called from vmError.cpp. I think the ARM version can be deleted - it was only put in place to add "experimental" to features used in the ARM build, but that has all been superseded now.

In thread.cpp you changed the comment:

! // is initially computed. See Abstract_VM_Version::vm_info_string(). ! // is initially computed. See VM_Version::vm_info_string().

but the code you are being referred to actually is defined in Abstract_VM_version::vm_info_string().

We now call VM_Version::foo() all the time instead of Abstract_VM_Version::foo() even though foo() is only "overridden" in a handful of cases, and sometimes there is no base version! I think in places we only ever want to have one definition of foo() - e.g. the main version functions:

static int vm_major_version() { return _vm_major_version; } static int vm_minor_version() { return _vm_minor_version; } static int vm_security_version() { return _vm_security_version; } static int vm_patch_version() { return _vm_patch_version; } static int vm_build_number() { return _vm_build_number; }

should not, in all probability, ever be "overridden". In that case we really should call them via Abstract_VM_Version::foo(). But there's no way to express methods that should not be "overridden". :( I think the real answer here (not for this change) is to handle VM_Version the way we handle the os class, and to only generate a single class containing both shared and platform-specific components. That might also allow us to do away with the "parallel work threads" mess:

static unsigned int nof_parallel_worker_threads(...); static unsigned int parallel_worker_threads(); static unsigned int calc_parallel_worker_threads();

Huh! ???

For this change if you address the vm_info_string issues I flagged that suffices - thanks.

I think I may adapt the JBS issue for handling VM_Version_ext so that it collapses all this into a single class.

Thanks, David

Open Webrev: http://cr.openjdk.java.net/~hseigel/bug7041262/webrev/index.html

JBS Bug:  https://bugs.openjdk.java.net/browse/JDK-7041262 The fix was tested by running Mach5 tiers 1 and 2 tests and builds on Linux-x64, Windows, and Mac OS X, running tiers 3-5 tests on Linux-x64, and by running JCK-12 Lang and VM tests on Linux-x64.  I also did test builds on zero, minimal, and aarch64. Thanks, Harold

More information about the hotspot-runtime-dev mailing list