RFR[10]: 8180614: Skip range and constraint checks on non-existent flags

[Gerard Ziemski]

Thank you Claes and Ioi for 2 very nice optimizations!

Looks good!

ps. Claes, did you run "hotspot/test/runtime/CommandLine/OptionsValidation” with this change?


cheers

> On May 19, 2017, at 9:44 AM, Claes Redestad <claes.redestad at oracle.com> wrote:
> 
> Hi Ioi,
> 
> On 2017-05-19 16:26, Ioi Lam wrote:
>> Hi Claes,
>> 
>> The new webrev looks good. Thanks!
> 
> Thanks!
> 
> For reference this gets down the cost of checking constraints and ranges from ~3.5M instructions (~1ms) to less than 200k (<0.1ms).
> 
> /Claes
> 
>> 
>> - Ioi
>> 
>> On 5/19/17 4:40 AM, Claes Redestad wrote:
>>> Hi Ioi,
>>> 
>>> On 05/18/2017 07:42 PM, Ioi Lam wrote:
>>>> Hi Claes,
>>>> 
>>>> 
>>>> This looks good to me.
>>> 
>>> thanks!
>>> 
>>>> 
>>>> As a further optimization, I wonder if we can get rid of all the calls
>>>> 
>>>> CommandLineFlagRangeList::check_ranges() -> Flag::find_flag()
>>> 
>>> Yes, I was considering approaches to improve this even further as a follow up, but it turns out it was easier than I had envisioned with some helpful pointers... ;-)
>>> 
>>>> 
>>>> Now, we will do more range checks than before, including flags that won't be returned by Flag::find_flag, like these guys:
>>>> 
>>>> Flag* Flag::find_flag(const char* name, size_t length, bool allow_locked, bool return_flag) {
>>>>  for (Flag* current = &flagTable[0]; current->_name != NULL; current++) {
>>>>    if (str_equal(current->_name, current->get_name_length(), name, length)) {
>>>>      ...
>>>>      if (!(current->is_unlocked() || current->is_unlocker())) {
>>>>        if (!allow_locked) {
>>>>          // disable use of locked flags, e.g. diagnostic, experimental,
>>>>          // commercial... until they are explicitly unlocked
>>>> HERE>>>>  return NULL;
>>>> 
>>>> 
>>>> But I think this is better, as we can even find errors in the ergo code that might have programmatically modified a diagnostic flag, for example.
>>> 
>>> Yes, this definitely seems like an improvement.
>>> 
>>> Updated webrev: http://cr.openjdk.java.net/~redestad/8180614/hotspot.01/
>>> 
>>> We discussed offline if all the constraint and range classes could be turned into templates, and while it superficially seems easy there are some uses of constraint methods from globals.cpp that needs special care, so I decided to defer that cleanup.
>>> 
>>> 
>>>> 
>>>> Or, we can even go one step further, add the ranges into the Flag::flags table directly. Then, we just need to walk this table sequentially. We can get rid of all the malloc inside CommandLineFlagRangeList::init() <-- how many cycles are spent here?
>>>> 
>>>> => try building commandLineFlagRangeList.o with --save-temps and look at the file commandLineFlagRangeList.ii :-)
>>>> 
>>>> 
>>>> And, we can sort Flag::flags alphabetically (by generating this table using a C program, which includes globals.hpp, during build time). That way Flag::find_flag can be searched on O(log n) time.
>>>> 
>>>> What do you think?
>>> 
>>> Might be a lot of effort for no measurable startup gain (since the above solution gets rid of calls to most calls to find_flag), and other uses of find_flag doesn't seem very performance critical.
>>> 
>>> Cleaning up the magic numbers in commandLineFlag*List.cpp would be nice, though...
>>> 
>>> Thanks!
>>> 
>>> /Claes
>>> 
>>>> 
>>>> 
>>>> Thanks
>>>> - Ioi
>>>> 
>>>> 
>>>> On 5/18/17 9:35 AM, Claes Redestad wrote:
>>>>> Hi,
>>>>> 
>>>>> analyzing the remaining overhead in command line flag
>>>>> checking after JDK-8178991, I found that we're taking a
>>>>> runtime cost scanning for flags that doesn't exist in the
>>>>> build, e.g., develop flags in a product build.
>>>>> 
>>>>> Since such flags are simply emitted as constants, they
>>>>> can't[1] be in violation of the constraints, thus I think
>>>>> we can safely skip such checks altogether.
>>>>> 
>>>>> This removes ~500k instructions from being executed for
>>>>> no reason during VM startup on a 64-bit JVM.
>>>>> 
>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8180614
>>>>> Webrev: http://cr.openjdk.java.net/~redestad/8180614/hotspot.00/
>>>>> 
>>>>> Thanks!
>>>>> 
>>>>> /Claes
>>>>> 
>>>>> [1] Unless the default value is invalid, which is a potential
>>>>> programmer error that would be caught in debug builds
>>>>> anyhow.
>>>> 
>>> 
>> 
>