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

[Ioi Lam]

Hi Claes,

The new webrev looks good. Thanks!

- 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.
>>
>