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