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

classic Classic list List threaded Threaded
9 messages Options
Reply | Threaded
Open this post in threaded view
|

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

Thomas Schatzl
Hi all,

  can I have reviews for this cleanup that changes the oop-misuse for
marking (setting LSB to 1 for array slices) into adding a
proper G1TaskQueueEntry that hides that mess a little?

CR:
https://bugs.openjdk.java.net/browse/JDK-8168467
Webrev:
http://cr.openjdk.java.net/~tschatzl/8168467/webrev/
Testing:
jprt, tier2+3 gc tests

Thanks,
  Thomas

Reply | Threaded
Open this post in threaded view
|

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

Kim Barrett
> On Feb 23, 2017, at 6:02 AM, Thomas Schatzl <[hidden email]> wrote:
>
> Hi all,
>
>   can I have reviews for this cleanup that changes the oop-misuse for
> marking (setting LSB to 1 for array slices) into adding a
> proper G1TaskQueueEntry that hides that mess a little?
>
> CR:
> https://bugs.openjdk.java.net/browse/JDK-8168467
> Webrev:
> http://cr.openjdk.java.net/~tschatzl/8168467/webrev/
> Testing:
> jprt, tier2+3 gc tests
>
> Thanks,
>   Thomas

------------------------------------------------------------------------------
src/share/vm/gc/g1/g1ConcurrentMark.hpp
  77     return (HeapWord*)((uintptr_t)_holder &~ ArraySliceBit);

"&~ ArraySliceBit" => "& ~ArraySliceBit"

------------------------------------------------------------------------------
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?

Or can the need for an assignment operator be eliminated?  That is,
replace one or both with a named function, say "assign(...)".

Note also that copy-assign w/o copy-construct is often considered a
bad smell.

------------------------------------------------------------------------------
src/share/vm/gc/g1/g1ConcurrentMark.hpp
  57   G1TaskQueueEntry(oop obj) : _holder(obj) { }
  58   G1TaskQueueEntry(HeapWord* addr) : _holder((void*)((uintptr_t)addr | ArraySliceBit)) { }

Should these really be implicit conversions?

------------------------------------------------------------------------------
src/share/vm/gc/g1/g1ConcurrentMark.hpp
  57   G1TaskQueueEntry(oop obj) : _holder(obj) { }

From usage elsewhere, I suspect obj must not be NULL.  If true, an
assert would be good.

------------------------------------------------------------------------------
src/share/vm/gc/g1/g1ConcurrentMark.hpp
 273   // Pushes the given buffer containing at most OopsPerChunk elements on the mark

OopsPerChunk is renamed.  There are other occurrences.

------------------------------------------------------------------------------
src/share/vm/gc/g1/g1ConcurrentMark.hpp
 278   bool par_push_chunk(G1TaskQueueEntry* buffer);
 283   bool par_pop_chunk(G1TaskQueueEntry* buffer);

Pre-existing: I think the API might be cleaner if these took OopChunk
arguments, rather than entry pointers.

------------------------------------------------------------------------------
src/share/vm/gc/g1/g1ConcurrentMark.cpp
2011   void operator()(G1TaskQueueEntry task_entry) const {
2012     guarantee(task_entry.is_array_slice() || task_entry.obj()->is_oop(),
...
2015     guarantee(task_entry.is_array_slice() || !_g1h->is_in_cset(task_entry.obj()),

Since these are guarantees, I think it would be more readable to hoist
the is_array_slice check.

------------------------------------------------------------------------------
src/share/vm/gc/g1/g1ConcurrentMark.cpp
2467     G1TaskQueueEntry obj;
...
2470       scan_object(obj);

Shouldn't "obj" be "entry".  And scan_object seems misnamed.

------------------------------------------------------------------------------
src/share/vm/gc/g1/g1ConcurrentMarkObjArrayProcessor.hpp
  53   // Process the given continuation "oop". Returns the number of words scanned.
  54   size_t process_slice(HeapWord* slice);

Comment needs updating.

------------------------------------------------------------------------------

Reply | Threaded
Open this post in threaded view
|

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

Thomas Schatzl
Hi Kim,

  sorry for the delay...

On Sat, 2017-02-25 at 14:06 -0500, Kim Barrett wrote:

> >
> > On Feb 23, 2017, at 6:02 AM, Thomas Schatzl <thomas.schatzl@oracle.
> > com> wrote:
> >
> > Hi all,
> >
> >   can I have reviews for this cleanup that changes the oop-misuse
> > for
> > marking (setting LSB to 1 for array slices) into adding a
> > proper G1TaskQueueEntry that hides that mess a little?
> >
> > CR:
> > https://bugs.openjdk.java.net/browse/JDK-8168467
> > Webrev:
> > http://cr.openjdk.java.net/~tschatzl/8168467/webrev/
> > Testing:
> > jprt, tier2+3 gc tests
> >
> > Thanks,
> >   Thomas
> -------------------------------------------------------------------
> src/share/vm/gc/g1/g1ConcurrentMark.hpp 
>   77     return (HeapWord*)((uintptr_t)_holder &~ ArraySliceBit);
>
> "&~ ArraySliceBit" => "& ~ArraySliceBit"

Fixed.

>
> -------------------------------------------------------------------
> 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.

> 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 kept this for now.

> Note also that copy-assign w/o copy-construct is often considered a
> bad smell.

I added that one, although it is the same as the default one.

> -------------------------------------------------------------------
> src/share/vm/gc/g1/g1ConcurrentMark.hpp 
>   57   G1TaskQueueEntry(oop obj) : _holder(obj) { }
>   58   G1TaskQueueEntry(HeapWord* addr) :
> _holder((void*)((uintptr_t)addr | ArraySliceBit)) { }
>
> Should these really be implicit conversions?

I think I fixed this by using factory methods and hidden constructors.

> -------------------------------------------------------------------
> src/share/vm/gc/g1/g1ConcurrentMark.hpp 
>   57   G1TaskQueueEntry(oop obj) : _holder(obj) { }
>
> From usage elsewhere, I suspect obj must not be NULL.  If true, an
> assert would be good.

Fixed.

> -------------------------------------------------------------------
> src/share/vm/gc/g1/g1ConcurrentMark.hpp 
>  273   // Pushes the given buffer containing at most OopsPerChunk
> elements on the mark
>
> OopsPerChunk is renamed.  There are other occurrences.

I did a lot of renaming in addition to that.

>
> -------------------------------------------------------------------
> ----------- 
> src/share/vm/gc/g1/g1ConcurrentMark.hpp 
>  278   bool par_push_chunk(G1TaskQueueEntry* buffer);
>  283   bool par_pop_chunk(G1TaskQueueEntry* buffer);
>
> Pre-existing: I think the API might be cleaner if these took OopChunk
> arguments, rather than entry pointers.

I would prefer if that were done as part of JDK-8162952 because it
seems an obvious change to do in that context.

> -------------------------------------------------------------------
> src/share/vm/gc/g1/g1ConcurrentMark.cpp
> 2011   void operator()(G1TaskQueueEntry task_entry) const {
> 2012     guarantee(task_entry.is_array_slice() || task_entry.obj()-
> >is_oop(),
> ...
> 2015     guarantee(task_entry.is_array_slice() || !_g1h-
> >is_in_cset(task_entry.obj()),
>
> Since these are guarantees, I think it would be more readable to
> hoist
> the is_array_slice check.

Fixed.

>
> --------------------------------------------------------------------
> src/share/vm/gc/g1/g1ConcurrentMark.cpp
> 2467     G1TaskQueueEntry obj;
> ...
> 2470       scan_object(obj);
>
> Shouldn't "obj" be "entry".  And scan_object seems misnamed.

Fixed. Also renamed other methods with "obj" getting entries.

>
> ------------------------------------------------------------------
> src/share/vm/gc/g1/g1ConcurrentMarkObjArrayProcessor.hpp
>   53   // Process the given continuation "oop". Returns the number of
> words scanned.
>   54   size_t process_slice(HeapWord* slice);
>
> Comment needs updating.

Fixed.

New webrev:
http://cr.openjdk.java.net/~tschatzl/8168467/webrev.0_to_1/ (incrementa
l)
http://cr.openjdk.java.net/~tschatzl/8168467/webrev.1/ (full)

Thanks,
  Thomas

Reply | Threaded
Open this post in threaded view
|

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

Kim Barrett
> On Mar 7, 2017, at 11:35 AM, Thomas Schatzl <[hidden email]> 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.
>
>> 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

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?

> New webrev:
> http://cr.openjdk.java.net/~tschatzl/8168467/webrev.0_to_1/ (incrementa
> l)
> http://cr.openjdk.java.net/~tschatzl/8168467/webrev.1/ (full)

Other than the operator= overload question, looks good.


Reply | Threaded
Open this post in threaded view
|

Re: 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@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

Reply | Threaded
Open this post in threaded view
|

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

Kim Barrett
> On Mar 8, 2017, at 5:31 AM, Thomas Schatzl <[hidden email]> 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@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.

Reply | Threaded
Open this post in threaded view
|

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

Thomas Schatzl
Hi,

On Thu, 2017-03-09 at 16:28 -0500, Kim Barrett wrote:
> >
> > On Mar 8, 2017, at 5:31 AM, Thomas Schatzl <[hidden email]
> > 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.)
>
> 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.

Thanks,
  Thomas

Reply | Threaded
Open this post in threaded view
|

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

sangheon.kim@oracle.com
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 <[hidden email]
>>> 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 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,
Sangheon


>
> Thanks,
>    Thomas
>

Reply | Threaded
Open this post in threaded view
|

Re: 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@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