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

David Holmes david.holmes at oracle.com
Mon Aug 17 02:07:59 UTC 2020


Cesar, Thomas,

As this is no longer a simple code refactoring I'm withdrawing from the 
review process. I'm also about to go on a weeks vacation.

Cheers,
David

On 16/08/2020 7:12 pm, Thomas Stüfe wrote:
> Hi Cesar,
> 
> This is going in the right direction, but needs some more thinking.
> 
> (ccing gc since gc devs may want to look at the argument parsing, see 
> remarks below)
> 
> ---
> 
> In my last mail I was kind of rough-sketching my idea, and did not count 
> on you taking it over verbatim, sorry. The assert I quoted had a number 
> of errors:
> 
> 1) it missed the assert message. Therefore building your patch gets us 
> build errors. For the prototypes of assert, see utilities/debug.hpp.
> 
> 2) The assert I quoted was wrong since it also fires for special == 
> false && _fd == -1 which is a valid combo. So, after building we now get 
> a crash right away.
> 
> Allowed combinations:
> 
> !special && _fd == -1 (anonymous, non-large-paged, this is 99% of 
> standard cases)
> special && _fd == -1  (anonymous large paged memory)
> !special && _fd != -1  (mapping over a file)
> 
> Disallowed:
> special == true && _fd != -1  (mapping over a file with large pages)
> 
> So, the assert should be:
> 
> assert(_fd == -1 || special == false, "wrong arguments")
> 
> or perhaps clearer to read:
> 
> ---------------
> 
> 3) 
> https://cr.openjdk.java.net/~adityam/cesar/8244801/2/src/hotspot/share/gc/shared/gcArguments.cpp.udiff.html
> 
> Unfortunately argument parsing is more complicated and now raises 
> additional questions. None of those are not the fault of your patch, 
> which only moves logic out of ReservedSpace to arg parsing.
> 
> +   if (!FLAG_IS_DEFAULT(AllocateHeapAt) && UseLargePages) {
> +     jio_fprintf(defaultStream::error_stream(),
> +       "AllocateHeapAt and UseLargePages cannot be used together.\n");
> +     status = false;
> +   }
> 
> 3.1) You disallow using AllocateHeapAt and UseLargePages at the same 
> time. You use !FLAG_IS_DEFAULT(AllocateHeapAt), similar to code above 
> and below. But I think it is irrelevant whether the user did explicitly 
> specify that flag. If you were to change the default value for 
> AllocateHeapAt to a non-NULL value - for whatever weird reason - it 
> would still be wrong.
> 
> I think you can just remove the FLAG_IS_DEFAULT and fall back to:
> 
> - if (!FLAG_IS_DEFAULT(AllocateHeapAt) && UseLargePages) {
> + if (AllocateHeapAt != NULL && UseLargePages) {
> 
> And I think the same logic applies to AllocateOldGenAt, and affects the 
> other code in this function.
> 
> 3.2) There is a tiny difference now as to how we treat TPH.
> 
> Large pages come, at linux, in two flavours: UseHugeTLBFS and 
> UseTransparentHugePages. The former are explicit pinned pages, the 
> latter uses TPH. When UseTransparentHugePages is active, 
> os::can_commit_large_page_memory() returns true (actually, this is the 
> only case on all platforms where this is true).
> 
> So, the patch causes AllocateHeapAt to be disallowed also for 
> transparent huge pages. Before, that combination would be allowed. Now 
> it's forbidden.
> 
> But I am not sure whether this combination is in any way relevant, or if 
> it could even work. Especially on persistent memory which was the reason 
> AllocateHeapAt was introduced. Digging into gcArguments.cpp, I believe 
> it is not considered relevant, since at lines 63ff, I find:
> 
>    if (!FLAG_IS_DEFAULT(AllocateOldGenAt)) {
>   .....
>      // When AllocateOldGenAt is set, we cannot use largepages for 
> entire heap memory.
>      // Only young gen which is allocated in dram can use large pages, 
> but we currently don't support that.
>      FLAG_SET_DEFAULT(UseLargePages, false);  <--
>    }
> 
> which unconditionally switches off large pages for AllocateOldGenAt. 
> Since AllocateOldGetAt is very similar to AllocateHeapAt, disallowing 
> large pages for AllocateHeapAt - TPH or not - should be also fine.
> 
> 3.3) which brings me to the next question: should we disallow this flag 
> combination and exit with an error, like your patch does now, or should 
> we quietly disable large pages instead and go on, like lines 63ff does 
> for AllocateOldGenAt?
> 
> I believe the latter would be better, but I leave this to gc people to 
> answer.
> 
> -------------------
> 
> Finally, a jtreg test may be good, testing the expected behaviour (which 
> kind of depends on the answer to question 3.3). For examples, see the 
> various tests at test/hotspot/jtreg/gc/arguments.
> 
> -----
> 
> Sorry if this balloons a bit. I think this cleanup is very worthwhile.
> 
> Cheers, Thomas
> 
> 
> 
> 
> 
> 
> 
> On Sat, Aug 15, 2020 at 10:03 PM Cesar Soares Lucas 
> <Divino.Cesar at microsoft.com <mailto:Divino.Cesar at microsoft.com>> wrote:
> 
>     Hi David/Thomas,
> 
>     Please see updated webrev here:
>     https://cr.openjdk.java.net/~adityam/cesar/8244801/2/
> 
>     - I added a validation for `AllocateHeapAt and UseLargePages` in
>        `src/hotspot/share/gc/shared/gcArguments.cpp` because there was
>     some similar
>        validation there. Please advise if this isn't the best place to
>     put such validation.
> 
>     - I renamed `_fd_for_heap` to `_fd` in `virtualspace.*` following
>     your suggestion
>        in the previous e-mail.
> 
>     - Refactored the code in `virtualspace.*` according to the previous
>     e-mail and the
>        changes above.
> 
> 
>     Thanks for looking into this!
>     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 12, 2020 10:41 AM
>     *To:* Thomas Stüfe <thomas.stuefe at gmail.com
>     <mailto:thomas.stuefe at gmail.com>>; David Holmes
>     <david.holmes at oracle.com <mailto:david.holmes at oracle.com>>
>     *Cc:* hotspot-dev developers <hotspot-dev at openjdk.java.net
>     <mailto:hotspot-dev at openjdk.java.net>>; 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:* Re: RFR: 8244801 - Code duplication in
>     ReservedSpace::initialize/ReservedHeapSpace::try_reserve_heap
>     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
>     <mailto:thomas.stuefe at gmail.com>>
>     Sent: August 11, 2020 11:14 PM
>     To: David Holmes <david.holmes at oracle.com
>     <mailto:david.holmes at oracle.com>>
>     Cc: Cesar Soares Lucas <Divino.Cesar at microsoft.com
>     <mailto:Divino.Cesar at microsoft.com>>; hotspot-dev developers
>     <hotspot-dev at openjdk.java.net
>     <mailto:hotspot-dev at openjdk.java.net>>; 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: 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><mailto: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><mailto: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><mailto: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><mailto: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><mailto:Brian.Stafford at microsoft.com
>     <mailto:Brian.Stafford at microsoft.com>>>; Aditya Mandaleeka
>     <adityam at microsoft.com
>     <mailto:adityam at microsoft.com><mailto:adityam at microsoft.com
>     <mailto:adityam at microsoft.com>>>
>     > Subject: RFR: 8244801 - Code duplication in ReservedSpace::initialize/ReservedHeapSpace::try_reserve_heap
>     >
>     >
>     > Bug: https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugs.openjdk.java.net%2Fbrowse%2FJDK-8244801&data=02%7C01%7Cdivino.cesar%40microsoft.com%7C0c4400b1693449eecd1008d83ee98678%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637328519987544935&sdata=Hw08jb5yVYI%2B8FlMzlziE2FVxBKUWe7qBNwOGgRN1%2FU%3D&reserved=0<https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugs.openjdk.java.net%2Fbrowse%2FJDK-8244801&data=02%7C01%7Cdivino.cesar%40microsoft.com%7C0c4400b1693449eecd1008d83ee98678%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637328519987544935&sdata=Hw08jb5yVYI%2B8FlMzlziE2FVxBKUWe7qBNwOGgRN1%2FU%3D&reserved=0
>     <https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugs.openjdk.java.net%2Fbrowse%2FJDK-8244801&data=02%7C01%7Cdivino.cesar%40microsoft.com%7C0c4400b1693449eecd1008d83ee98678%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637328519987544935&sdata=Hw08jb5yVYI%2B8FlMzlziE2FVxBKUWe7qBNwOGgRN1%2FU%3D&reserved=0%3Chttps://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugs.openjdk.java.net%2Fbrowse%2FJDK-8244801&data=02%7C01%7Cdivino.cesar%40microsoft.com%7C0c4400b1693449eecd1008d83ee98678%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637328519987544935&sdata=Hw08jb5yVYI%2B8FlMzlziE2FVxBKUWe7qBNwOGgRN1%2FU%3D&reserved=0>>
>     > Webrev: 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%7C0c4400b1693449eecd1008d83ee98678%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637328519987544935&sdata=%2FVhUkQP5FAcGscqqMz3TrllcnRfT1PzMGdgLUAJKX5c%3D&reserved=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%7C0c4400b1693449eecd1008d83ee98678%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637328519987544935&sdata=%2FVhUkQP5FAcGscqqMz3TrllcnRfT1PzMGdgLUAJKX5c%3D&reserved=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%7C0c4400b1693449eecd1008d83ee98678%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637328519987544935&sdata=%2FVhUkQP5FAcGscqqMz3TrllcnRfT1PzMGdgLUAJKX5c%3D&reserved=0%3Chttps://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%7C0c4400b1693449eecd1008d83ee98678%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637328519987544935&sdata=%2FVhUkQP5FAcGscqqMz3TrllcnRfT1PzMGdgLUAJKX5c%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
>     >
> 


More information about the hotspot-gc-dev mailing list