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

[Thomas Schatzl]

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.

Thanks,
  Thomas