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

[Claes Redestad]

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