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