RFR: JDK-8257588: Make os::_page_sizes a bitmask [v4]

[Stefan Karlsson]

On Fri, 4 Dec 2020 06:05:10 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 Stefan K

I think this mostly looks good, but there are a number of cleanups that I'd like you to consider.

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

> 1850: ////// Implementation of PageSizes
> 1851: 
> 1852: void os::PageSizes::add(size_t pagesize) {

pagesize => page_size and the same for other places as well.

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

> 1874:     return 0;
> 1875:   }
> 1876:   int l = exact_log2(pagesize) + 1;

l is not a good variable name. It's too easy to mistake for 1, depending on font.

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

> 1892: size_t os::PageSizes::smallest() const {
> 1893:   assert(min_page_size() > 0, "Sanity");
> 1894:   return next_larger(min_page_size() / 2);

I'm not sure why you use min_page_size() / 2 instead of another small enough power_of_2, say the smallest one:
return next_larger(1);
and thereby get rid of the dependency on the odd min_page_size().

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

> 124:   static OSThread*          _starting_thread;
> 125:   static address            _polling_page;
> 126:   static PageSizes        _page_sizes;

Indentation is off by two spaces

test/hotspot/gtest/runtime/test_os.cpp line 612:

> 610: static const int max_page_size_log2 = (int)(sizeof(size_t) * 8);
> 611: 
> 612: TEST_VM(os, pagesizeset_test_range) {

pagesizeset name needs to be updated.

test/hotspot/gtest/runtime/test_os.cpp line 613:

> 611: 
> 612: TEST_VM(os, pagesizeset_test_range) {
> 613:   for (int bit = min_page_size_log2; bit < max_page_size_log2; bit ++) {

bit ++ => bit++ : the same in the rest of the file.

test/hotspot/gtest/runtime/test_os.cpp line 610:

> 608: 
> 609: static const int min_page_size_log2 = exact_log2(os::min_page_size());
> 610: static const int max_page_size_log2 = (int)(sizeof(size_t) * 8);

Could this be simply BitsPerWord?

test/hotspot/gtest/runtime/test_os.cpp line 617:

> 615:       const size_t s =  (size_t)1 << bit;
> 616:       const size_t s2 = (size_t)1 << bit2;
> 617:       //tty->print_cr(SIZE_FORMAT " - " SIZE_FORMAT, s, s2);

Remove?

test/hotspot/gtest/runtime/test_os.cpp line 625:

> 623:       }
> 624:       ASSERT_EQ((size_t)0, pss.smallest());
> 625:       ASSERT_EQ((size_t)0, pss.largest());

This doesn't have to be inside a double nested for loop. This could be single check done once.

test/hotspot/gtest/runtime/test_os.cpp line 669:

> 667:   stringStream ss(buffer, sizeof(buffer));
> 668:   pss.print_on(&ss);
> 669:   // tty->print_cr("%s", buffer);

Remove?

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

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