RFR: 8287215: Add method to scan bitmap backwards [v3]

[Thomas Schatzl]

On Fri, 27 May 2022 02:32:47 GMT, Kim Barrett <kbarrett at openjdk.org> wrote:

>> Thomas Schatzl has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Minor simplification
>
> src/hotspot/share/utilities/bitMap.hpp line 327:
> 
>> 325:   static constexpr idx_t NotFound = ~(idx_t)0;
>> 326:   // Find the index of the first bit set scanning from r_index (inclusive) to
>> 327:   // l_index (inclusive) backwards.
> 
> I don't think r_index should be inclusive.  I think that's surprising (such
> ranges are usually half-open, and always so elsewhere in this class).  (It
> caught me by surprise; I failed to notice that in the doc and was quite
> confused when reviewing the implementation until I realized what was going
> on.)  I also think it makes these operations harder to use. Having a closed
> range forces the caller to deal with the unrepresentable empty range
> explicitly.  A common use-case for such searches is to iterate over the bits;
> the inclusive r_index makes that more awkward for the caller.  Obviously this
> affects everything else in this change, so I haven't looked at the code very
> much yet.

I see your point, and I'm not against that change.

In ZGC code the equivalent method also seems `r_index`-inclusive. @stefank , any opinions?

> src/hotspot/share/utilities/bitMap.hpp line 328:
> 
>> 326:   // Find the index of the first bit set scanning from r_index (inclusive) to
>> 327:   // l_index (inclusive) backwards.
>> 328:   // Returns that index or NotFound if there is no such bit in the range.
> 
> I think that, for consistency, it might be good for the forward search to also
> return NotFound when appropriate. That's something that should be done as a
> followup though, I think.

I agree that this should not be done here - this will cause quite a few additional changes I think.

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

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