RFR: 8151539: Remove duplicate AlwaysTrueClosures

[Stefan Karlsson]

Hi reviewers,

Are you OK with the latest patch?

Thanks,
StefanK

On 10/03/16 09:58, Stefan Karlsson wrote:
> Hi Kim,
>
> Updated webrevs:
>  http://cr.openjdk.java.net/~stefank/8151539/webrev.02.delta/
>  http://cr.openjdk.java.net/~stefank/8151539/webrev.02/
>
> I moved the AlwaysTrueClosure to iterator.hpp, and created a stack 
> local instance in jniHandles.cpp.
>
> I also found that there's an AlwaysFalseClosure, so I took the liberty 
> of moving this to iterator.hpp as well.
>
> Just a few comments inlined:
>
> On 2016-03-10 00:11, Kim Barrett wrote:
>>> On Mar 9, 2016, at 3:01 PM, Stefan Karlsson 
>>> <stefan.karlsson at oracle.com> wrote:
>>>
>>> Hi Mikael,
>>>
>>> On 2016-03-09 20:39, Mikael Gerdin wrote:
>>>> Hi Stefan,
>>>>
>>>> On 2016-03-09 17:44, Stefan Karlsson wrote:
>>>>> Hi all,
>>>>>
>>>>> Please review this patch to remove a bunch of redundant
>>>>> AlwaysTrueClosure classes.
>>>>>
>>>>> http://cr.openjdk.java.net/~stefank/8151539/webrev.00/
>>>> There is still
>>>> 2128 // This should be moved to the shared markSweep code!
>>>> 2129 class PSAlwaysTrueClosure: public BoolObjectClosure {
>>>> 2130 public:
>>>> 2131   bool do_object_b(oop p) { return true; }
>>>> 2132 };
>>>> 2133 static PSAlwaysTrueClosure always_true;
>>>> in psParallelCompact.cpp
>>>>
>>>> Otherwise the change looks good, I don't need to see an updated 
>>>> webrev.
>>> Great that you saw this!
>>>
>>> I've uploaded a new webrev, to make it easier for the second reviewer:
>>> http://cr.openjdk.java.net/~stefank/8151539/webrev.01/
>> A couple of things below; otherwise looks good.  I don't need a new
>> webrev for these.
>>
>> ------------------------------------------------------------------------------ 
>>
>> src/share/vm/runtime/jniHandles.cpp
>>   136   static AlwaysTrueClosure always_true;
>>
>> Function-scoped static initialization is not thread-safe in C++03.
>
> Thanks for pointing that out. It's not a problem here though, but I'll 
> clean this out to prevent future bugs our questions.
>
>>
>> I don't think there's any noticable benefit to a static here even with
>> thread-safe initialization, as the construction is probably pretty
>> simple. (It might be simple enough that there's no actual
>> thread-safety issue, but why take that chance.)
>
> I agree. I've changed it to a stack instance.
>
>>
>> ------------------------------------------------------------------------------ 
>>
>> src/share/vm/gc/shared/referenceProcessor.cpp
>>   269   class AlwaysAliveClosure: public BoolObjectClosure {
>>
>> This local class can go away too.
>
> Done.
>
> Thanks,
> StefanK
>
>>
>> ------------------------------------------------------------------------------ 
>>
>> src/share/vm/gc/shared/genCollectedHeap.cpp
>> [removed]
>>   688 class AlwaysTrueClosure: public BoolObjectClosure {
>>
>> src/share/vm/gc/parallel/psMarkSweep.cpp
>> [removed]
>>   574 class PSAlwaysTrueClosure: public BoolObjectClosure {
>>
>> src/share/vm/gc/parallel/psParallelCompact.cpp
>> [removed]
>> 2129 class PSAlwaysTrueClosure: public BoolObjectClosure {
>>
>> Nice!  Fortunately, these aren't technically ODR violations, since the
>> same-named definitions are identical.
>>
>> ------------------------------------------------------------------------------ 
>>
>>
>