RFR (M): 8168467: Use TaskEntry as task mark queue elements

[Kim Barrett]

> On Mar 8, 2017, at 5:31 AM, Thomas Schatzl <thomas.schatzl at oracle.com> wrote:
> 
> Hi,
> 
> On Tue, 2017-03-07 at 19:11 -0500, Kim Barrett wrote:
>>> 
>>> On Mar 7, 2017, at 11:35 AM, Thomas Schatzl <thomas.schatzl at oracle.
>>> com> wrote:
>>>> 
>>>> src/share/vm/gc/g1/g1ConcurrentMark.hpp 
>>>>   42 #ifdef _MSC_VER
>>>>   43 #pragma warning(push)
>>>>   44 // warning C4522: multiple assignment operators specified
>>>>   45 #pragma warning(disable:4522)
>>>>   46 #endif
>>>> 
>>>> Is the non-volatile overload of operator= needed?
>>> Yes, it is part of the API for the task queues.
> 
> Actually, the volatile overload is part of the task queue API.
> 
>>>> 
>>>> Or can the need for an assignment operator be eliminated?  That
>>>> is, replace one or both with a named function, say "assign(...)".
>>> What is the reason for this suggestion? Having two assignment
>>> operators, one for volatile and other for non-volatile instances
>>> does not seem to be really problematic. When I tried this the code
>>> doing the assignments became more unnatural looking.
>>> 
>>> This seems to be more a case of the compiler being over-cautious
>>> than a real problem.
>> I have a vague recollection of a real problem involving having both
>> volatile and nonvolatile overloads with the Microsoft compiler under
>> some circumstances.  But that could be old information, or I could
>> just be misremembering.  The documentation for this warning does
>> indicate that it's only informational, and the code should work as
>> expected
> 
> We use that code for seven years in Hotspot (StarTask in taskqueue.hpp)
> without issues.
> 
> [The differences between this and StarTask is another issue, now with
> having implemented all your suggestions regarding improved code quality
> in G1TaskQueueEntry, they now look sufficiently different to be not
> immediately recognizable to mostly serve the same purpose. I filed 	
> JDK-8176362]
> 
>> The warning, and the need to suppress it, is still annoying.  That's
>> why I asked whether the nonvolatile overload is actually
>> required.  Is there any problem with just having the volatile method?
> 
> It simply looks ugly and unnatural to use for accommodating some weird
> compiler spewing out otherwise informational messages as warnings.
> 
> I prepared a patch for how this would look like:
> http://cr.openjdk.java.net/~tschatzl/8168467/webrev.1_to_2/
> http://cr.openjdk.java.net/~tschatzl/8168467/webrev.2/
> 
> I will probably need to add a comment now why we use an extra assign
> method instead of the operator now :-)
> 
> As for the change, if you insist on having an extra assign method just
> to avoid this pragma and decrease readability of the uses of that code,
> let's use webrev.2 as the current version.

I agree the assign functioning approach is kind of ugly.

But you still haven't answered my question as to whether the
nonvolatile operator is actually needed, e.g. just use the volatile
operator always.

But I finally remembered this:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=7614
(Sorry it took so long to dredged this up, but it's been maybe a dozen
years since I tripped over it.)

Discussion there suggests returning volatile& is problematic, which
seems right. The callers (GenericTaskQueue's push and push_slow) need
to const_cast away the volatile and then cast to void the result. Much
simpler would be an operation that didn't return volatile& (either a
void returning volatile operator= or a named function).

This is all getting rather far afield from the change at hand though.
I suggest going with webrev.1 and a cleanup RFE around volatile operator=.
(I suggest no volatile operator=, instead a volatile assign function,
and change taskqueue push to use that.)

I think StarTask and maybe the various flavors of oop are related.
Hm, why doesn't the oop class need similar warning suppression?

If you agree, then webrev.1 looks good to me.