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

[Thomas Schatzl]

Hi,

On Tue, 2017-03-14 at 17:40 -0700, sangheon wrote:
> Hi Thomas,
> 
> On 03/10/2017 05:01 AM, Thomas Schatzl wrote:
> > 
> > Hi,
> > 
> > On Thu, 2017-03-09 at 16:28 -0500, Kim Barrett wrote:
> > > 
> > > > 
> > > > On Mar 8, 2017, at 5:31 AM, Thomas Schatzl <thomas.schatzl at orac
> > > > le.c
> > > > om> wrote:
> > > > 
> > > > Hi,
> > > > 
> > > > On Tue, 2017-03-07 at 19:11 -0500, Kim Barrett wrote:
> > [...]
> > > 
> > > > 
> > > > 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.
> > I guess I simply did not understand your question (or the
> > motivation
> > for your question)...
> > 
> > > 
> > > 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.)
> Just a question.
> Do you agree to Kim filing a cleanup RFE around volatile operator= ?

I can't tell yet :) I think it could be looked at, but I am not sure it
is worth the effort just to fix an imho weird compiler warning.
That change sounds to be somewhat limited in scope too, so it might
turn out a good idea after all.

> > > 
> > > I think StarTask and maybe the various flavors of oop are
> > > related.
> > > Hm, why doesn't the oop class need similar warning suppression?
> > I found that if CHECK_UNHANDLED_OOPS is enabled (to compile the
> > relevant code in oopsHierarchy.hpp), there is a global pragma to
> > disable this warning.
> > 
> > > 
> > > If you agree, then webrev.1 looks good to me.
> > > 
> > Agree. Let's keep webrev.1.
> webrev.1 seems good to me too.
> 
> Just minor comments, so I don't need additional webrev if you agree
> to 
> change.
> src/share/vm/gc/g1/g1ConcurrentMark.hpp
> 
> 233   TaskQueueEntryChunk* _base;               // Bottom address of 
> allocated memory area.
> Please align  comment with other lines. "// Bottom" line has more
> spaces 
> than line 232 and 234.

Thanks for your review. I will fix the spaces.

Thanks,
  Thomas