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

[sangheon]

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