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

[Ioi Lam]

Hi Claes,


This looks good to me.

As a further optimization, I wonder if we can get rid of all the calls

CommandLineFlagRangeList::check_ranges() -> Flag::find_flag()

This can be done by changing

void emit_range_intx(const char* name, int min, intx max) {
   CommandLineFlagRangeList::add(new CommandLineFlagRange_int(name, min, 
max));
}

=>

void emit_range_intx(const char* name, const intx* ptr, int min, intx max) {
   CommandLineFlagRangeList::add(new CommandLineFlagRange_int(name, ptr, 
min, max));
}

class CommandLineFlagRange_intx {
   const int* _ptr; ....

- Flag::Error check_int(int value, bool verbose = true) {
+ Flag::Error check_int(bool verbose = true) {
+ int value = *_ptr
       ......
}


Then, the main loop can look like this:

   for (int i=0; i<length(); i++) {
     CommandLineFlagRange* range = at(i);
-   const char* name = range->name();
-   Flag* flag = Flag::find_flag(name, strlen(name), true, true);
-   // We must check for NULL here as lp64_product flags on 32 bit 
architecture
-   // can generate range check (despite that they are declared as 
constants),
-   // but they will not be returned by Flag::find_flag()
-   if (flag != NULL) {
-     if (flag->is_int()) {
-       int value = flag->get_int();
-       if (range->check_int(value, true) != Flag::SUCCESS) status = false;
+   switch(range->type()) {
+   case CommandLineFlagRange::INT
+       if (range->check_int(true) != Flag::SUCCESS) status = false;

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.

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?


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.