RFR (M): 8138983: Runtime: implement ranges for Shared*** flags

Wed Nov 11 17:22:11 UTC 2015

hi Jiangli,

Thank you very much for the review. Please see my answers in-line:

On 11/10/2015 07:19 PM, Jiangli Zhou wrote:
> Hi Gerard,
>
> The changes looks good. Thank you for moving all Shared*** flags' values in one place! I have a few comments below.
>
> For the MIN_SHARED_MISC_DATA_SIZE & MIN_SHARED_MISC_CODE_SIZE, the original check in metaspace.cpp align the computed
> values to os::vm_allocation_granularity(). Any reason why the alignment requirement is removed for the new values
> defined in metaspaceShared.hpp?

The range function compares the min with the value before any alignment takes place, so we're good (we're not mixing 
comparison of aligned vs unaligned values here), but good question. The values later do get aligned when assigned locally.

>
> For the following MAX values, it seems they should be calculated using
> the MIN_SHARED_READ_WRITE_SIZE, MIN_SHARED_READ_ONLY_SIZE, MIN_SHARED_MISC_DATA_SIZE and MIN_SHARED_MISC_CODE_SIZE
> instead of the DEFAULT values since the runtime flags’ setting can be smaller than the default values? Does that sound
> reasonable?
>
> 71 #define MAX_SHARED_READ_WRITE_SIZE
> (MAX_SHARED_DELTA-(DEFAULT_SHARED_READ_ONLY_SIZE+DEFAULT_SHARED_MISC_DATA_SIZE+DEFAULT_SHARED_MISC_CODE_SIZE)-SHARED_PAGE)
> 72 #define MAX_SHARED_READ_ONLY_SIZE
> (MAX_SHARED_DELTA-(DEFAULT_SHARED_READ_WRITE_SIZE+DEFAULT_SHARED_MISC_DATA_SIZE+DEFAULT_SHARED_MISC_CODE_SIZE)-SHARED_PAGE)
> 73 #define MAX_SHARED_MISC_DATA_SIZE
> (MAX_SHARED_DELTA-(DEFAULT_SHARED_READ_WRITE_SIZE+DEFAULT_SHARED_READ_ONLY_SIZE+DEFAULT_SHARED_MISC_CODE_SIZE)-SHARED_PAGE)
> 74 #define MAX_SHARED_MISC_CODE_SIZE
> (MAX_SHARED_DELTA-(DEFAULT_SHARED_READ_WRITE_SIZE+DEFAULT_SHARED_READ_ONLY_SIZE+DEFAULT_SHARED_MISC_DATA_SIZE)-SHARED_PAGE)

I have to think about this for a bit, but I think that we can do this using constraints, but then we will have to 
disable these flags in Dmitry's testing framework, because it will be possible to create an arrangement where the range 
(with MAX needing to be set to 0x7FFFFFFF) will pass, but not the constraint. Such arrangement will make VM exit with 
code 2 (ie. not crash), which would be reported as failure by the testing framework.

>
> You probably have already done it, it would be good to verify the changes pass all the test in
> hotspot/test/runtime/SharedArchiveFile on all platforms.

Oh yes, I did verify with all hotspot tests run on all platforms.

cheers

>
> Thanks,
>
> Jiangli
>
>> On Nov 10, 2015, at 9:31 AM, gerard ziemski <gerard.ziemski at oracle.com <mailto:gerard.ziemski at oracle.com>> wrote:
>>
>> hi all,
>>
>> I have updated the fix with rev2, incorporating feedback from David:
>>
>> - Move the metaspace constants back into metaspace code
>> - Fix test framework that specifies jsa file to be used by the "-Xshare:dump" flag tests
>>
>> http://cr.openjdk.java.net/~gziemski/8138983_rev2
>>
>>
>> cheers
>>
>>
>> On 11/06/2015 03:33 PM, gerard ziemski wrote:
>>> hi all,
>>>
>>> I have updated the fix with rev1, incorporating feedback from Dmitry:
>>>
>>> - Fix comments and indentation
>>> - Add "-XShare:dump" while testing Shared* flags
>>> - Temporarily disable testing of SharedMiscDataSize as with current min default it exits the VM with code 2 (which is
>>> allowed by the JEP, but not the test framework). This issue is tracked by JDK-8141650
>>>
>>> http://cr.openjdk.java.net/~gziemski/8138983_rev1
>>>
>>>
>>>
>>> cheers
>>>
>>>
>>>
>>> On 11/06/2015 09:58 AM, gerard ziemski wrote:
>>>>
>>>> hi all,
>>>>
>>>> Please review this final fix for JEP-245
>>>>
>>>> We implement the remaining runtime flags in the Shared* family.
>>>>
>>>> Summary of changes:
>>>>
>>>> #1 core fix - we factor out the constants to be used in the ranges
>>>>
>>>> src/share/vm/classfile/compactHashtable.cpp
>>>> src/share/vm/memory/metaspace.cpp
>>>> src/share/vm/memory/metaspaceShared.hpp
>>>> src/share/vm/runtime/globals.hpp
>>>>
>>>>
>>>> #2 we increase default sizes of range and constraint tables to minimize mallocs
>>>>
>>>> src/share/vm/runtime/commandLineFlagConstraintList.cpp
>>>> src/share/vm/runtime/commandLineFlagRangeList.cpp
>>>>
>>>>
>>>> #3 we fix a bug that I run into the OptionsValidation test framework while working
>>>>
>>>> test/runtime/CommandLine/OptionsValidation/common/optionsvalidation/IntJVMOption.java
>>>>
>>>>
>>>> #4 we add functionality to OptionsValidation test framework that we later use
>>>>
>>>> test/runtime/CommandLine/OptionsValidation/common/optionsvalidation/JVMOptionsUtils.java
>>>>
>>>>
>>>> #5 we cleanup and extend the existing test to detect a case where we get out of range value
>>>>
>>>> test/runtime/SharedArchiveFile/LimitSharedSizes.java
>>>>
>>>>
>>>> The fix passes RBT "hotspot/test/:hotspot_all,testlist,noncolo.testlist --add-tonga-keyword quick” and JPRT “hotspot"
>>>> tests
>>>>
>>>> References:
>>>>
>>>>     bug link: https://bugs.openjdk.java.net/browse/JDK-8138983
>>>>  open webrev: http://cr.openjdk.java.net/~gziemski/8138983_rev0
>>>>
>>>>
>>>> hg_stat:
>>>>
>>>>  src/share/vm/classfile/compactHashtable.cpp                                              |    2 +-
>>>>  src/share/vm/memory/metaspace.cpp                                                        |   30 --
>>>>  src/share/vm/memory/metaspaceShared.hpp                                                  |   19 +-
>>>>  src/share/vm/runtime/commandLineFlagConstraintList.cpp                                   |    2 +-
>>>>  src/share/vm/runtime/commandLineFlagRangeList.cpp                                        |    2 +-
>>>>  src/share/vm/runtime/globals.hpp                                                         |   63 +++++-
>>>>  test/runtime/CommandLine/OptionsValidation/common/optionsvalidation/IntJVMOption.java    |    4 +-
>>>>  test/runtime/CommandLine/OptionsValidation/common/optionsvalidation/JVMOptionsUtils.java |   81 +++++++
>>>>  test/runtime/SharedArchiveFile/LimitSharedSizes.java                                     |  180 +++++++++-------
>>>>  9 files changed, 242 insertions(+), 141 deletions(-)
>>>
>