[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-hotspot-runtime-dev
Subject:    Re: RFR (L/S) manual cleanup of white space issues (8057109)
From:       "Daniel D. Daugherty" <daniel.daugherty () oracle ! com>
Date:       2014-09-09 20:53:40
Message-ID: 540F68D4.2080603 () oracle ! com
[Download RAW message or body]

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


[prev in list] [next in list] [prev in thread] [next in thread]