RFR (L/S) manual cleanup of white space issues (8057109)

Daniel D. Daugherty daniel.daugherty at oracle.com
Tue Sep 9 20:53:40 UTC 2014


On 9/9/14 1:36 PM, Coleen Phillimore wrote:
>
> Hi Dan,
>
> In general, doing things like whitespace cleanups makes merging new 
> fixes that need to be backported for some more difficult, but it 
> appears that the cleanups that you've chosen not interfere with 
> backports of components other than the locking subsystems.  So I like 
> this change.

Thanks! The other fix (8057107) and this fix (8057109) should
be the last of the large cleanups motivated by the Contended
Locking project. I have to say that cleaning this stuff, while
it turned out to be more difficult than I expected, will be
worth it down the road...


> The only exception to these changes that I think might interfere with 
> other changes to other components:
>
> http://cr.openjdk.java.net/~dcubed/8057109-webrev/1-jdk9-hs-rt/src/share/vm/runtime/thread.cpp.udiff.html 
>
>
> -    jint res = Atomic::cmpxchg(strong_roots_parity, &_oops_do_parity, 
> thread_parity);
> +    jint res = Atomic::cmpxchg(strong_roots_parity, &_oops_do_parity,
> +                               thread_parity);

Reverted lines 787-8.


> -void JavaThread::oops_do(OopClosure* f, CLDClosure* cld_f, 
> CodeBlobClosure* cf) {
> +void JavaThread::oops_do(OopClosure* f, CLDClosure* cld_f,
> +                         CodeBlobClosure* cf) {

Reverted lines 2666-7.


> -void CompilerThread::oops_do(OopClosure* f, CLDClosure* cld_f, 
> CodeBlobClosure* cf) {
> +void CompilerThread::oops_do(OopClosure* f, CLDClosure* cld_f,
> +                             CodeBlobClosure* cf) {

Reverted lines 3189-90.


>
>
> -void Threads::possibly_parallel_oops_do(OopClosure* f, CLDClosure* 
> cld_f, CodeBlobClosure* cf) {
> +void Threads::possibly_parallel_oops_do(OopClosure* f, CLDClosure* 
> cld_f,
> +                                        CodeBlobClosure* cf) {

Reverted lines 4061-2.

>
> -  assert(!is_par ||
> -         (SharedHeap::heap()->n_par_threads() ==
> -         SharedHeap::heap()->workers()->active_workers()), "Mismatch");
> +  assert(!is_par || (SharedHeap::heap()->n_par_threads() ==
> + SharedHeap::heap()->workers()->active_workers()),
> +                     "Mismatch");

Reverted lines 4073-75.


> These might interfere with GC changes and these are really only to fit 
> the line into 80 columns which, even though it's mine and your 
> preference, isn't part of the coding standard.
>
> So I think these 5 should be reverted.

While I tend to think of src/share/vm/runtime/thread.cpp
as belonging to the Runtime team, These are clearly GC
related code so I reverted those changes.


> The rest look great.

Thanks!

Dan


>
> Coleen
>
> On 9/8/14, 6:01 PM, Daniel D. Daugherty wrote:
>> Updated after Fred's code review:
>>
>> Here is the URL for the (L)arge webrev:
>>
>> http://cr.openjdk.java.net/~dcubed/8057109-webrev/1-jdk9-hs-rt/
>>
>> Here is the URL for the (S)mall webrev which was generated with a 
>> version
>> of webrev that ignores all whitespace changes:
>>
>> http://cr.openjdk.java.net/~dcubed/8057109-webrev/1-jdk9-hs-rt-no_ws/
>>
>> Dan
>>
>>
>> On 9/7/14 1:13 PM, Daniel D. Daugherty wrote:
>>> Greetings,
>>>
>>> I have the fix for the following bug ready for JDK9 RT_Baseline:
>>>
>>>     JDK-8057109 manual cleanup of white space issues prior to Contended
>>>                 Locking reorder and cache line bucket
>>>     https://bugs.openjdk.java.net/browse/JDK-8057109
>>>
>>> This is both a (L)arge and a (S)mall review! Here is the URL for the
>>> (L)arge webrev:
>>>
>>> http://cr.openjdk.java.net/~dcubed/8057109-webrev/0-jdk9-hs-rt/
>>>
>>> Since these are mostly white space fixes, I recommend the udiff links
>>> instead of the frames links. Of course, some folks prefer the patch 
>>> file
>>> itself for white space fixes.
>>>
>>> Here is the URL for the (S)mall webrev which was generated with a 
>>> version
>>> of webrev that ignores all whitespace changes:
>>>
>>> http://cr.openjdk.java.net/~dcubed/8057109-webrev/0-jdk9-hs-rt-no_ws/
>>>
>>> Again, I recommend the udiff links since that's the fastest way to see
>>> that webrev_no_ws found minimal non-white space changes. The patch file
>>> for this webrev shows all the changes.
>>>
>>> These manual changes were motivated by various review comments made for
>>> earlier fixes in the Contended Locking project. During those reviews, I
>>> deferred making some of the requested format changes to this round of
>>> manual changes. In most cases, the reviewers posted comments with one
>>> or two examples of things they did not like and I've swept all 17 files
>>> that we've targeted for cleanup in the Contended Locking project 
>>> looking
>>> for those style problems and fixing them. I've made a total of 12 
>>> passes
>>> over the 17 files, but I cannot say that I've caught everything. Most
>>> stuff that annoys people is fixed, but it's very difficult to catch and
>>> fix them all without a real parser based solution.
>>>
>>> Thanks, in advance, for any comments, questions or suggestions.
>>>
>>> Dan
>>>
>>>
>>>
>>>
>>
>



More information about the hotspot-runtime-dev mailing list