RFR: JDK-8257588: Make os::_page_sizes a bitmask [v3]
Stefan Karlsson
stefank at openjdk.java.net
Thu Dec 3 21:28:59 UTC 2020
On Thu, 3 Dec 2020 13:59:11 GMT, Thomas Stuefe <stuefe at openjdk.org> wrote:
>> Hi,
>>
>> may I please have reviews for the following very small improvement:
>>
>> While discussing JDK-8243315 [1], and aiming to make planned changes like JDK-8256155 [2] easier:
>>
>> size_t os::_page_sizes[os::page_sizes_max];
>>
>> is an array used to keep all page sizes the hotspot can use. It is sorted by size and filled in at initialization time.
>>
>> Coding dealing with this can be simplified by making this a set (which is very easy since all page sizes are power-2-values so they lend themselves nicely to a bitmap).
>>
>> That has the following advantages:
>> - makes adding new sizes simple since we do not have to re-sort the array. Coding is easier to read too.
>> - it makes it possible to encode a set of page sizes in one number, so we can hand a set-of-page-sizes around as a value.
>>
>> -----
>>
>> The patch adds a new class, os::PagesizeSet, which is a bitmap containing page sizes. It adds gtests for this class. It replaces the old os::_page_sizes with an object of that class. It also removes an unused function.
>>
>>
>> Testing:
>> - nightlies at SAP
>> - manual tests with UseLargePages
>> - the new gtests
>> - gh actions (with x86 still failing because of unrelated issues)
>>
>> Thank you,
>>
>> Thomas
>>
>> [1] https://bugs.openjdk.java.net/browse/JDK-8243315
>> [2] https://bugs.openjdk.java.net/browse/JDK-8256155
>
> Thomas Stuefe has updated the pull request incrementally with one additional commit since the last revision:
>
> Feedback Thomas
src/hotspot/share/runtime/os.cpp line 1859:
> 1857: bool os::PagesizeSet::is_set(size_t pagesize) const {
> 1858: assert(is_power_of_2(pagesize), "pagesize must be a power of 2: " INTPTR_FORMAT, pagesize);
> 1859: return (_v & pagesize) > 0;
Why is this `> 0` and not simply `!= 0`
src/hotspot/share/runtime/os.cpp line 1869:
> 1867: return round_down_power_of_2(v2);
> 1868: }
> 1869: return 0;
In next_larger you use the for:
if (v2 == 0) {
return 0;
}
return ...
I think it would be nice to be consistent between the two functions.
src/hotspot/share/runtime/os.cpp line 1863:
> 1861:
> 1862: size_t os::PagesizeSet::next_smaller(size_t pagesize) const {
> 1863: assert(is_power_of_2(pagesize), "pagesize must be a power of 2: " INTPTR_FORMAT, pagesize);
You're using INTPR_FORMAT to print a size_t. I guess this is done to print hex values. We have a SIZE_FORMAT_HEX for that use case.
src/hotspot/share/runtime/os.hpp line 110:
> 108: PagesizeSet() : _v(0) {}
> 109: void add(size_t pagesize);
> 110: bool is_set(size_t pagesize) const;
Could this be private. It's an implementation detail that you use a bit set and have "set" bits. If not, maybe consider a more high-level name. is_added, is_registered, contains?
src/hotspot/share/runtime/os.hpp line 106:
> 104: // A simple value class holding a set of page sizes (similar to sigset_t)
> 105: class PagesizeSet {
> 106: uintx _v;
All in-parameters are size_t. It seems like this could be size_t as well.
-------------
PR: https://git.openjdk.java.net/jdk/pull/1522
More information about the hotspot-dev
mailing list