[8u] RFR: 8144483: One long Safepoint pause directly after each GC log rotation

[Yumin Qi]

Cheleswer,

   If the call to check_addr0() is unnecessary, remove it is a better 
choice I think. In fatal error, means VM will exit, the error handler 
will print memory info, so it is redundant here. I don't know why it is 
added to log rotation code in the first place.

   Thanks
   Yumin

On 1/6/2016 3:37 AM, cheleswer sahu wrote:
> Hi,
>
> Please review the code changes for 
> "https://bugs.openjdk.java.net/browse/JDK-8144483".
>
> webrev link: http://cr.openjdk.java.net/~kevinw/8144483/webrev.00/
> JBS link: https://bugs.openjdk.java.net/browse/JDK-8144483
>
> Bug brief:
> A long pause is observed after each gc log rotation in Solaris.
>
> Problem Identified:
> In each GC log rotation "print_memory_info()" is getting called 
> through dump_loggc_header().
> "print_memory_info()" of solaris version calls check_addr0(st);
>
> File: 
> http://hg.openjdk.java.net/jdk8u/jdk8u/hotspot/file/80959a760b85/src/os/solaris/vm/os_solaris.cpp
>
> void os::print_memory_info(outputStream* st) {
>   st->print("Memory:");
>
>   st->print(" %dk page", os::vm_page_size()>>10);
>
>   st->print(", physical " UINT64_FORMAT "k", os::physical_memory()>>10);
>
>   st->print("(" UINT64_FORMAT "k free)", os::available_memory() >> 10);
>
>   st->cr();
>  (void) check_addr0(st);
> }
>
> Now check_addr0(st) function do a lot of read call to read the data 
> from /proc/self/map.
> and check if virtual address is mapped to 0x0. These read calls take 
> lot of time which results in GC rotation pause.
> Here calling check_addr0() seems unnecessary for every log rotation. 
> It will be more logical if this function gets called only
> when an error is reported.
>
> Solution proposed:
> Before GC log rotation print_memory_info() is ever getting called from 
> Vm_error.cpp during error reporting. And in case of error reporting
> checking for address mapping to 0x0 looks fine. So the proposed 
> solution is to do an extra check inside print_memory_info().
>
> -  (void) check_addr0(st);
> +  if (VMError::fatal_error_in_progress()){
> +     (void) check_addr0(st);
> +  }
>  }
>
>  This check doesn't fit well inside printing function, but at this 
> point I don't see the need to create a new os:: method and change all 
> the OS classes just for that check.
>
> Regards,
> Cheleswer