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

[Claes Redestad]

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