RFR: 8027545: Improve object array chunking test in G1's copy_to_survivor_space [v2]

[Thomas Schatzl]

Hi,

On 12.09.20 03:02, Kim Barrett wrote:
>> On Sep 11, 2020, at 8:50 AM, Thomas Schatzl <thomas.schatzl at oracle.com> wrote:
>>
>> Hi,
>>
>> On 11.09.20 11:01, Kim Barrett wrote:
>> [...]
>>> Kim Barrett has updated the pull request incrementally with one additional commit since the last revision:
>>>    Respond to reviews by ayang and sjohanss.
>>>
>>> Changes:
>>>    - all: https://git.openjdk.java.net/jdk/pull/90/files
>>>    - new: https://git.openjdk.java.net/jdk/pull/90/files/9397514e..1981c392
>>> Webrevs:
>>>   - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=90&range=01
>>>   - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=90&range=00-01
>>>    Stats: 152 lines in 6 files changed: 122 ins; 14 del; 16 mod
>>>    Patch: https://git.openjdk.java.net/jdk/pull/90.diff
>>>    Fetch: git fetch https://git.openjdk.java.net/jdk pull/90/head:pull/90
>>> PR: https://git.openjdk.java.net/jdk/pull/90
>>
>>   - partialArrayTaskStepper.hpp:
>>
>> s/_task_fannout/_task_fanout (or maybe _task_fan_out)
> 
> I thought it looked a little odd at first, but never got around to spell
> checking it, and after a while it looked fine by familiarity. Will fix
> ("fanout").
> 
>>   - since _task_fannout and _task_limit are only assigned in the constructor of PartialArrayTaskStepper, maybe make them const?
> 
> I'd rather not.  I've been down that road and didn't like the results. Just
> as an example, doing that sort of thing renders a class copyable but not
> assignable, swappable, or (in some cases) moveable from.  That's an odd
> combination. I already (though recently) made start() and next() const.
> 
>>   - also the asserts in PartialArrayTaskStepper::next_impl could be moved to the constructor initialization list but otoh the fannout > 0 assert does make the calculation below immediately obviously safe.
> 
> As you say, the current location shows the requirements for the nearby
> calculations.
> 
>> Looks good otherwise.

Okay. Going to mark this as reviewed assuming the rename will be done.

Thanks,
   Thomas