RFR (S/M) expose L1_data_cache_line_size for diagnostic/sanity checks (8049717)

[Daniel D. Daugherty]

On 7/14/14 1:15 PM, Vladimir Kozlov wrote:
> Dan,
>
> Only code style comments.
>
> Can you use one line for next statements in objectMonitor.cpp.?:
>
> +    tty->print_cr("INFO: sizeof(ObjectMonitor)=" SIZE_FORMAT,
> +        sizeof(ObjectMonitor));

The line would be 85 chars long - I try to stay below 80.


>
> +  if (verbose) tty->print_cr("INFO: L1_data_cache_line_size=%u",
> +                   cache_line_size);

The line would be 82 chars long - I try to stay below 80.


> +  guarantee(error_cnt == 0,
> +      "Fatal error(s) found in ObjectMonitor::sanity_checks()");

The line would be 86 chars long - I try to stay below 80.


> +  ObjectSynchronizer::sanity_checks(verbose, cache_line_size, 
> &error_cnt,
> +      &warning_cnt);

The line would be 88 chars long - I try to stay below 80.


> And in synchronizer.cpp:
>
> +    tty->print_cr("INFO: sizeof(SharedGlobals)=" SIZE_FORMAT,
> +        sizeof(SharedGlobals));
>
> We usually start second line of a print statement where "(" on first 
> line:
>
> +      tty->print_cr("WARNING: the _header and _owner fields are closer "
> +                    "than a cache line which permits false sharing.");

I've actually seen a variety of different styles for indentation
for "continuation lines". In particular, I've been looking at
formatting issues for all of the monitor code because of the wide
range on styles in that code.

At this point, I'm strongly leaning toward double the normal
indentation from the beginning of the statement that is being
continued.

So in this example:

     if ((sizeof(ObjectMonitor) % cache_line_size) != 0) {
       tty->print_cr("WARNING: ObjectMonitor size is not a multiple of "
           "a cache line which permits false sharing.");
       warning_cnt++;
     }

the string continuation is indented 2x2 spaces.

Another example:

   if (verbose) tty->print_cr("INFO: L1_data_cache_line_size=%u",
                    cache_line_size);

the parameter is indented 2x2 spaces from the tty->... statement
and not from the if-statement.

This function definition is another example:

void ObjectSynchronizer::sanity_checks(const bool verbose,
          const uint cache_line_size, int *error_cnt_ptr,
          int *warning_cnt_ptr) {
   u_char *addr_begin      = (u_char*)&GVars;

which makes it much easier to see the parameters versus the locals.
In this case, I've indented 2x2 spaces from the start of the
function name; I'm not 100% happy with this one. In Solaris OS code,
this would have been easier:

void
ObjectSynchronizer::sanity_checks(const bool verbose,
     const uint cache_line_size, int *error_cnt_ptr,
     int *warning_cnt_ptr) {
   u_char *addr_begin      = (u_char*)&GVars;

since all function names start on the left-edge... :-)

This is particularly useful when you have a really long
while-statement:

   while ((this_is_some_field & some_mask) == some_bit_value ||
       (this_is_some_field & some_other_mask) == some_bit_value2) {
     this_is_a_statement_in_the_while_block;
   }

so the continuation of the while-statement is clearly indented
relative to the statements in the while-block body.

I'm not planning to make any changes based on these format
comments; please let me know if you're OK with that.

Dan




>
> Thanks,
> Vladimir
>
> On 7/14/14 10:47 AM, Daniel D. Daugherty wrote:
>> Greetings,
>>
>> I've tweaked the fix based on David H's feedback.
>>
>> Here is the URL for the third round webrev:
>>
>> http://cr.openjdk.java.net/~dcubed/8049717-webrev/2-jdk9-hs-rt/
>>
>> The following files changed between round 1 and round 2:
>>
>> src/share/vm/runtime/objectMonitor.cpp
>> src/share/vm/runtime/synchronizer.cpp
>>
>> Thanks, in advance, for any comments, questions or suggestions.
>>
>> Dan
>>
>>
>> On 7/11/14 2:14 PM, Daniel D. Daugherty wrote:
>>> Greetings,
>>>
>>> I've tweaked the fix based on Vladimir's feedback.
>>>
>>> Here is the URL for the second round webrev:
>>>
>>> http://cr.openjdk.java.net/~dcubed/8049717-webrev/1-jdk9-hs-rt/
>>>
>>> The following files changed between round 0 and round 1:
>>>
>>> src/cpu/sparc/vm/vm_version_sparc.cpp
>>> src/cpu/x86/vm/vm_version_x86.cpp
>>> src/cpu/x86/vm/vm_version_x86.hpp
>>> src/share/vm/runtime/objectMonitor.cpp
>>> src/share/vm/runtime/synchronizer.cpp
>>>
>>> Thanks, in advance, for any comments, questions or suggestions.
>>>
>>> Dan
>>>
>>>
>>> On 7/9/14 10:42 AM, Daniel D. Daugherty wrote:
>>>> Greetings,
>>>>
>>>> I have the fix for the following bug ready for JDK9 RT_Baseline:
>>>>
>>>>     JDK-8049717 expose L1_data_cache_line_size for diagnostic/sanity
>>>>                 checks
>>>>     https://bugs.openjdk.java.net/browse/JDK-8049717
>>>>
>>>> Here is the URL for the webrev:
>>>>
>>>> http://cr.openjdk.java.net/~dcubed/8049717-webrev/0-jdk9-hs-rt/
>>>>
>>>> This fix is a standalone piece from my Contended Locking reorder
>>>> and cache-line bucket. I've split it off as an independent bug fix
>>>> in order to make the reorder and cache-line bucket more clear.
>>>>
>>>> Testing:
>>>>
>>>> - JPRT test jobs
>>>> - manual testing of the new output via existing options:
>>>>   -XX:+UnlockExperimentalVMOptions -XX:SyncKnobs=Verbose=1
>>>>   -XX:+ExecuteInternalVMTests -XX:+VerboseInternalVMTests
>>>> - Aurora Adhoc nsk.sajdi and vm.parallel_class_loading as part of
>>>>   testing for my Contended Locking reorder and cache-line bucket
>>>>
>>>> Thanks, in advance, for any comments, questions or suggestions.
>>>>
>>>> Dan
>>>>
>>>>
>>>>
>>>>
>>>
>>>
>>>
>>