RFR (M): 8168467: Use TaskEntry as task mark queue elements
sangheon.kim at oracle.com
Wed Mar 15 00:40:47 UTC 2017
On 03/10/2017 05:01 AM, Thomas Schatzl wrote:
> 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:
>>> On Tue, 2017-03-07 at 19:11 -0500, Kim Barrett wrote:
>>> I prepared a patch for how this would look like:
>>> 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:
>> (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
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.
More information about the hotspot-gc-dev