RFR: 8244801 - Code duplication in ReservedSpace::initialize/ReservedHeapSpace::try_reserve_heap

[Thomas Stüfe]

Man ReservedSpace really should get a revamp. It is a general purpose
device, but so polluted with GC/java heap assumptions :(

I think this can be done better. Let's look at it:

   // If OS doesn't support demand paging for large page memory, we need
   // to use reserve_memory_special() to reserve and pin the entire region.
   // If there is a backing file directory for this space then whether
   // large pages are allocated is up to the filesystem of the backing file.
   // So we ignore the UseLargePages flag in this case.
   bool special = large && !os::can_commit_large_page_memory();
   if (special && _fd_for_heap != -1) {
     special = false;
     if (UseLargePages && (!FLAG_IS_DEFAULT(UseLargePages) ||
       !FLAG_IS_DEFAULT(LargePageSizeInBytes))) {
       log_debug(gc, heap)("Ignoring UseLargePages since large page support
is up to the file system of the backing file for Java heap");
     }
   }

So, IIUC:

- Caller requested large pages (since large=true). Note that caller can be
anyone, does not have to be the one allocating java heap.
- The OS cannot commit large pages on demand (AFAICS only possible on Linux
TPH). So we need to explicitly reserve large paged pinned memory, which we
call "special" for some reason.
- But someone specified fd_for_heap (why "fd_for_heap"? This is a general
purpose API. Why so much GC/heap specific code?). So we deny the request
and feel this is worth an error message.

The error messages conflict: one says basically "we ignore this but the
underlying file system may be larged paged, so you still may be good, I
don't know". One says "We ignore this, period.".

So the problem worth reporting back to the user is that he specified
manually both -XX:+UseLargePages and -XX:AllocateHeapAt And that is
supposed to be a user error. It would be much clearer if that conflict
would be handled at argument parsing time, not here. Then this code could
just be:

bool special = large && !os::can_commit_large_page_memory();
assert(special || _fd_for_heap != -1);

That would be cleaner and remove at least some of the "it's used for heap
allocations" assumptions from ReservedSpace. We could even rename
"fd_for_heap" to a general "fd" and just make it a general argument (so,
independent of AllocateHeapAt). Because someone other than java heap might
want to reserve memory atop of a file.

Just my 5 cent. Sorry for the slight rant.

Cheers, Thomas



On Wed, Aug 12, 2020 at 6:42 AM David Holmes <david.holmes at oracle.com>
wrote:

> Hi Cesar,
>
> This seems fine to me and I can sponsor it for you once we have a second
> review.
>
> Thanks,
> David
>
> On 12/08/2020 12:27 pm, Cesar Soares Lucas wrote:
> > Gentle ping. Can anyone PTAL?
> >
> >
> > Thanks,
> > Cesar
> >
> > ________________________________
> > From: hotspot-dev <hotspot-dev-retn at openjdk.java.net> on behalf of
> Cesar Soares Lucas <Divino.Cesar at microsoft.com>
> > Sent: August 6, 2020 2:30 PM
> > To: hotspot-dev developers <hotspot-dev at openjdk.java.net>
> > Cc: Brian Stafford <Brian.Stafford at microsoft.com>; Aditya Mandaleeka <
> adityam at microsoft.com>
> > Subject: RFR: 8244801 - Code duplication in
> ReservedSpace::initialize/ReservedHeapSpace::try_reserve_heap
> >
> >
> > Bug: https://bugs.openjdk.java.net/browse/JDK-8244801
> > Webrev: https://cr.openjdk.java.net/~adityam/cesar/8244801/0/
> > Need sponsor: Yes
> >
> > Can you please review the above linked patch for the mentioned bug?
> > It's a small change that refactor a portion of duplicated code in
> `virtualspace.cpp`.
> >
> > I tried to structure the code so that the new code and the old printed
> the same
> > log messages. Please let me know if you want to simplify the method
> signature
> > and just hard code the log message. I also tried to come up with a short
> yet
> > descriptive name for the method; let me know if you have suggestions on
> that
> > as well.
> >
> >
> > Thanks,
> > Cesar
> > Software Engineer - Microsoft
> >
>