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

[Vladimir Kozlov]

If it was compiler related code I would object :)
But it is not, so I will agree with your formatting.

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