RFR: JDK-8257588: Make os::_page_sizes a set [v2]

[Thomas Schatzl]

On Thu, 3 Dec 2020 08:40:17 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:
> 
>   Fix 32bit issues

Changes requested by tschatzl (Reviewer).

To be really nitpicky: the subject of the PR should be made more specific to mention that the new set is a bitset not just a set. I.e. os::page_sizes has already previously been a set (of integers), so where's the change? :P

src/hotspot/share/runtime/os.cpp line 79:

> 77: int               os::_processor_count    = 0;
> 78: int               os::_initial_active_processor_count = 0;
> 79: os::PagesizeSet os::_page_sizes;

While I'm typically not a fan of aligning variable identifiers, it's already there. Please align this new one too with the others.

src/hotspot/share/runtime/os.cpp line 1851:

> 1849: 
> 1850: ////// Implementation of pagesizeset_t
> 1851: 

There is no pagesizeset_t ;)

src/hotspot/share/runtime/os.cpp line 1854:

> 1852: // A pagesizeset_t is a set containing a set of page sizes.
> 1853: 
> 1854: // sets the given page size

Not sure if this is the correct place for a description what the method does, it seems to me that the definition in the hpp file is a better place.

Also all other methods are described there already, and this description does not add anything new.

src/hotspot/share/runtime/os.cpp line 1863:

> 1861: bool os::PagesizeSet::is_set(size_t pagesize) const {
> 1862:   assert(is_power_of_2(pagesize), "pagesize must be a power of 2: " INTPTR_FORMAT, pagesize);
> 1863:   return _v & pagesize;

Please do not rely on implicit comparison against unequal 0.

src/hotspot/share/runtime/os.cpp line 1870:

> 1868:   assert(is_power_of_2(pagesize), "pagesize must be a power of 2: " INTPTR_FORMAT, pagesize);
> 1869:   // mask out all pages sizes >= pagesize:
> 1870:   uintx v2 = _v & pagesize - 1;

I would prefer to add the brackets instead of relying on operator strengths. They would make the intent more clear or at least require less thinking.

src/hotspot/share/runtime/os.cpp line 1904:

> 1902: size_t os::PagesizeSet::smallest() const {
> 1903:   assert(min_page_size() > 0, "Sanity");
> 1904:   return next_larger(min_page_size() / 2);

Why not just return `min_page_size()` here?

An assert may be added to check that they are the same though.

src/hotspot/share/runtime/os.hpp line 103:

> 101: 
> 102:  public:
> 103: 

The class PagesizeSet does not need to be public.

src/hotspot/share/runtime/os.cpp line 1860:

> 1858: }
> 1859: 
> 1860: // returns true if given page size is part of the set

Same as for `add()`.

src/hotspot/share/runtime/os.hpp line 112:

> 110:     bool is_set(size_t pagesize) const;
> 111:     // given a page size, return the next smaller page size in this set, or 0.
> 112:     size_t next_smaller(size_t pagesize) const;

Please let all documentation sentences start with upper case.

src/hotspot/share/runtime/os.cpp line 1919:

> 1917:     } else if (sz < G) {
> 1918:       st->print(SIZE_FORMAT "m", sz / M);
> 1919:     } else {

The prefixes for mega/gigabyte are "M" and "G" instead of "m" and "g" respectively

src/hotspot/share/runtime/os.cpp line 1887:

> 1885:     return 0;
> 1886:   }
> 1887:   while ((v2 & 1) == 0) {

Maybe the `count_trailing_zeros()` method could be used here instead of doing the same manually.

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

PR: https://git.openjdk.java.net/jdk/pull/1522Changes requested by tschatzl (Reviewer).