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

[Daniel D. Daugherty]

On 7/14/14 2:39 PM, Vladimir Kozlov wrote:
> If it was compiler related code I would object :)

And you should!


> But it is not, so I will agree with your formatting.

Thanks!

Originally, I had applied all formatting fixes to every file that the
Contended Locking project touched. I backed away from that when I
realized that you would rightfully string me up for messing with the
compiler file formatting! We'll still have to figure out how to
properly fix the formatting of Dice's code in your files, but that's
another day.

Dan


>
> Thanks,
> Vladimir
>
> On 7/14/14 1:07 PM, Daniel D. Daugherty wrote:
>> 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
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>>>
>>>>>
>>>>
>>