RFR: 8303621: BitMap::iterate should support lambdas and other function objects [v2]

[Stefan Karlsson]

On Mon, 6 Mar 2023 10:33:36 GMT, Kim Barrett <kbarrett at openjdk.org> wrote:

>> Please review this enhancement of BitMap::iterate to support lambdas and other
>> function objects as the operation being applied to the set bit indices. Some
>> for-loops using BitMap::get_next_one_offset have been changed to use
>> BitMap::iterate with a lambda.
>> 
>> (One reason for changing the for-loops is that I'm considering a change to the
>> get_next_one_offset API, and reducing the number of direct uses will simplify
>> that.)
>> 
>> For convenience, the function can either return void (always iterate over the
>> whole range) or bool (stop iteration if returns false).  Iteration using
>> closure objects invoked via a do_bit member function are now implemented by
>> being wrapped in a lambda, so get the same convenience.  (Though, of course,
>> if the closure is derived from BitMapClosure then do_bit returns bool.)
>> 
>> The unit tests are written as "early" tests, not requiring an initialized VM.
>> They also avoid any C heap allocation (even though C heap allocation has very
>> early support). This is done to minimize the requirements for running the
>> tests, since BitMap is used in a lot of places. This attempts to run these
>> tests before uses. (Yes, I know about JDK-8257226; maybe that will be fixed
>> someday.) (Some existing BitMap gtests should be modified to do similarly; see
>> JDK-8303636.)
>> 
>> Testing:
>> mach5 tier1-7
>
> Kim Barrett has updated the pull request incrementally with one additional commit since the last revision:
> 
>   improve description of iterate

Thanks for doing this change. This aligns well with what we currently have in the Generational ZGC repository.

I have one style comment that I think could help the readability of the code.

src/hotspot/share/utilities/bitMap.inline.hpp line 273:

> 271:     }
> 272:   }
> 273: }

IMHO, this is an eyesore that I prefer if it could be hidden away a bit. Could we create an `invoke` function that that calls this, and change the code to:

template <typename Function>
inline bool BitMap::iterate(Function function, idx_t beg, idx_t end) const {
  for (idx_t index = beg; true; ++index) {
    index = get_next_one_offset(index, end);
    if (index >= end) {
      return true;
    } else if (!invoke(function, index)) {
      return false;
    }
  }
}


And also add a comment why we need IterateInvoker?

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

Changes requested by stefank (Reviewer).

PR: https://git.openjdk.org/jdk/pull/12876