RFR: 8261859: gc/g1/TestStringDeduplicationTableRehash.java failed with "RuntimeException: 'Rehash Count: 0' found in stdout" [v2]

[Stefan Johansson]

On Mon, 1 Mar 2021 17:28:59 GMT, Kim Barrett <kbarrett at openjdk.org> wrote:

>> src/hotspot/share/gc/shared/stringdedup/stringDedupTable.cpp line 410:
>> 
>>> 408:     // Compute new size.  We can't shrink by more than a factor of 2,
>>> 409:     // because the partitioning for parallelization doesn't support more.
>>> 410:     if (size > _min_size) size /= 2;
>> 
>> I would prefer braces :) But an even nicer solution would be to use `clamp(...)` both for the grow and shrink case. What do you think about that?
>
> I think clamp just confuses things because only one of the bounds matters,
> which bound depending on whether growing or shrinking. And on the grow side
> the prior test against _max_size is still needed to eliminate any questions
> about overflow. That is, I wouldn't want to just use
> size = clamp(round_up_power_of_2(needed), _min_size, _max_size);
> without the prior check, because the round_up could perhaps overflow (at
> least in theory, and maybe only on 32bit platforms).  But with the prior
> check the clamp is superfluous.

I see, being sure there is no overflow is good (even if it feels very theoretical in this case). Otherwise I was thinking something like: 
if (grow) {
  size = round_up_power_of_2(needed);
} else if (shrink) {
  size = size / 2;
}
clamp(size, _min_size, _max_size);

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

PR: https://git.openjdk.java.net/jdk/pull/2769