[jdk8u-dev] RFR: 6899049: G1: Clean up code in ptrQueue.[ch]pp and ptrQueue.inline.hpp [v3]

[Andrew John Hughes]

On Thu, 21 Sep 2023 14:13:01 GMT, Sun Jianye <jianyesun at openjdk.org> wrote:

>> Hi all,
>> 
>> This pull request contains a backport of commit [b256989eb34a32c8f03be448c0645baeb5192a01](https://github.com/openjdk/jdk11u-dev/commit/b256989eb34a32c8f03be448c0645baeb5192a01) from the [openjdk/jdk11u-dev](https://github.com/openjdk/jdk11u-dev) repository.
>> 
>> As reported by issue :  https://bugs.openjdk.org/browse/JDK-8316278 .  We found  the indexing method of PtrQueue's buf  is not  correct  when  converting an integer of type size_t to type int, then calling the method PtrQueue::byte_index_to_index . 
>> The key problem is this way of using: 
>> 
>> size_t i=0;    _buf[byte_index_to_index((int)i)] = NULL;  
>> 
>> The variable i of size_t type  cannot be converted directly to an int type . Other than that, the return value of the function byte_index_to_index is the index of the array _buf, and it should be non-negative. So it  should be a type of size_t. 
>> Currently we have found 2 issues related to this problem,  https://bugs.openjdk.org/browse/JDK-8308169 and  https://bugs.openjdk.org/browse/JDK-8303961.  They are all triggered by a special  size number of buf, like '-XX:G1UpdateBufferSize=512M'  or  '-XX:G1SATBBufferSize=500m'
>> We also added a test case.
>> Please review this PR. Thanks.
>
> Sun Jianye has updated the pull request incrementally with one additional commit since the last revision:
> 
>   delete the test

A few minor unwanted differences have crept into the backport, but otherwise now looks mostly clean.

As to the test, if you do think it is worthwhile, please consider submitting it to openjdk/jdk.

hotspot/src/share/vm/gc_implementation/g1/dirtyCardQueue.cpp line 316:

> 314:     DirtyCardQueue& dcq = t->dirty_card_queue();
> 315:     if (dcq.size() != 0) {
> 316:       void **buf = dcq.get_buf();

The 11u version changes this to `void** buf`. Was there a reason for the difference here?

hotspot/src/share/vm/gc_implementation/g1/ptrQueue.hpp line 79:

> 77:   // Initialize this queue to contain a null buffer, and be part of the
> 78:   // given PtrQueueSet.
> 79:   PtrQueue(PtrQueueSet* qset, bool permanent  = false, bool active = false);

An extra space has sneaked in here between `permanent` and the `=` sign.

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

Changes requested by andrew (Reviewer).

PR Review: https://git.openjdk.org/jdk8u-dev/pull/374#pullrequestreview-1643146248
PR Review Comment: https://git.openjdk.org/jdk8u-dev/pull/374#discussion_r1336493277
PR Review Comment: https://git.openjdk.org/jdk8u-dev/pull/374#discussion_r1336493682