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

[Cesar Soares Lucas]

Thanks for reviewing this David and Thomas.

Thomas, the error log messages were also confusing to me. I'll refactor the code according to your suggestion and get back to you.


________________________________
From: Thomas Stüfe <thomas.stuefe at gmail.com>
Sent: August 11, 2020 11:14 PM
To: David Holmes <david.holmes at oracle.com>
Cc: Cesar Soares Lucas <Divino.Cesar at microsoft.com>; hotspot-dev developers <hotspot-dev at openjdk.java.net>; Brian Stafford <Brian.Stafford at microsoft.com>; Aditya Mandaleeka <adityam at microsoft.com>
Subject: Re: RFR: 8244801 - Code duplication in ReservedSpace::initialize/ReservedHeapSpace::try_reserve_heap

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<mailto: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<mailto:hotspot-dev-retn at openjdk.java.net>> on behalf of Cesar Soares Lucas <Divino.Cesar at microsoft.com<mailto:Divino.Cesar at microsoft.com>>
> Sent: August 6, 2020 2:30 PM
> To: hotspot-dev developers <hotspot-dev at openjdk.java.net<mailto:hotspot-dev at openjdk.java.net>>
> Cc: Brian Stafford <Brian.Stafford at microsoft.com<mailto:Brian.Stafford at microsoft.com>>; Aditya Mandaleeka <adityam at microsoft.com<mailto: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<https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugs.openjdk.java.net%2Fbrowse%2FJDK-8244801&data=02%7C01%7CDivino.Cesar%40microsoft.com%7C11fc0ac786f04942626108d83e870c0c%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637328097031716312&sdata=iH0BgOKWEWDdbdSTkNGd%2FN6BWfVMECVj%2B3vxKpV9dsA%3D&reserved=0>
> Webrev: https://cr.openjdk.java.net/~adityam/cesar/8244801/0/<https://nam06.safelinks.protection.outlook.com/?url=https:%2F%2Fcr.openjdk.java.net%2F~adityam%2Fcesar%2F8244801%2F0%2F&data=02%7C01%7CDivino.Cesar%40microsoft.com%7C11fc0ac786f04942626108d83e870c0c%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637328097031726312&sdata=8GU0CPIn6Nwt%2FLkYsquwtgrhFw4FtlOkhaw6ZX5gEfU%3D&reserved=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
>