RFR: 8243326: Cleanup use of volatile in taskqueue code

Ivan Walulya ivan.walulya at oracle.com
Wed Apr 29 10:04:47 UTC 2020


Looks good.

Minor:
Can you please modify some of the comments 

- “task queue.hpp:227 // Return true if the TaskQueue contains/does not contain any tasks.”
   Not sure what the comment is supposed to convey

- “taskqueue.inline.hpp:131  // We lose; a completing pop_global gets the element.  But the queue is empty”
  'completing' should have been ‘competing’

- Comments on the pop_global corner case can also be more elaborate, otherwise, it is not clear when “A pop_global operation may read an element that is being concurrently written by a push operation"


//Ivan

> On 22 Apr 2020, at 13:04, Kim Barrett <kim.barrett at oracle.com> wrote:
> 
>> On Apr 22, 2020, at 6:06 AM, Thomas Schatzl <thomas.schatzl at oracle.com> wrote:
>> 
>> Hi,
>> 
>> On 22.04.20 10:49, Kim Barrett wrote:
>>> Please review this change that eliminates most uses of the volatile
>>> qualifier in the taskqueue code.  The remaining uses are in the
>>> declarations of the queue's _bottom and _age members.  That's just our
>>> current style of tagging "atomic" variables manipulated by multiple
>>> threads.
>> 
>> Some minor nits:
>> 
>> - although I understand it is quite old code, maybe change MOD_N_MASK to CamelCase in some way as it is a static const now. I am okay to ignore this remark.
> 
> So far as I can tell, we don't have a consistent naming convention for
> constants. It's easy to find examples for all of CamelCase, snake_case,
> and UPPER_CASE.  I was also trying not to do too many extraneous changes.
> 
>> - pre-existing: static-assert that N is a power of 2 otherwise the masking won't work.
>> 
>> - pre-existing: N should also be smaller than what is representable by idx_t.
> 
> Done.  is_power_of_2 is not (yet) constexpr, so had to be manually inlined.
> 
>> - pre-existing: there are a few places/asserts where "N - 1" is used verbatim. Maybe exchange with that static above, however I'm not sure that unless it is named properly this is an improvement. Or have another constant for that. Or have a predicate/method for that always same-checking. Probably it's best to ignore this comment.
> 
> Something to do with underflow?  UNDERFLOW_SIZE, UnderflowSize?
> 
>> Looks good otherwise.
> 
> Thanks.
> 
> I have at least one more cleanup to go here anyway: JDK-8243325.  I
> could add some more for the constant names.
> 
> I prototyped a substantial restructuring of the classes, but decided
> not to mix that in with the other changes, if I pursued it at all.
> The use of templates in the current code is pretty clumsy.  For
> example, the MEMFLAGS parameter affects *everything*, even though very
> little code actually depends on it.  This may lead to a significant
> amount of code bloat.
> 
> I'm going to hold off on posting an updated webrev to see if there are
> any other comments.
> 




More information about the hotspot-gc-dev mailing list