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

Harold David Seigel harold.seigel at oracle.com
Tue Oct 16 14:08:30 UTC 2018

Hi David,

Thanks for reviewing this and for catching the ARM32 issue.  I'll make the vm_info_string() changes before pushing the change.

Harold

On 10/16/2018 1:14 AM, David Holmes wrote:

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 vminfostring() changes. On ARM32 we have an "overriding" version of VMVersion::VMinfostring() 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 AbstractVMVersion::vminfostring(). !   // is initially computed. See VMVersion::vminfostring(). but the code you are being referred to actually is defined in AbstractVMversion::vminfostring(). --- We now call VMVersion::foo() all the time instead of AbstractVMVersion::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 vmmajorversion()               { return vmmajorversion; } static int vmminorversion()               { return vmminorversion; } static int vmsecurityversion()            { return vmsecurityversion; } static int vmpatchversion()               { return vmpatchversion; } static int vmbuildnumber()                { return vmbuildnumber; } should not, in all probability, ever be "overridden". In that case we really should call them via AbstractVMVersion::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 VMVersion 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 nofparallelworkerthreads(...); static unsigned int parallelworkerthreads(); static unsigned int calcparallelworkerthreads(); Huh! ??? --- For this change if you address the vminfostring issues I flagged that suffices - thanks. I think I may adapt the JBS issue for handling VMVersionext 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