RFR: 8244801 - Code duplication in ReservedSpace::initialize/ReservedHeapSpace::try_reserve_heap
Thomas Stüfe
thomas.stuefe at gmail.com
Sun Aug 16 09:12:47 UTC 2020
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> 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> on behalf of
> Cesar Soares Lucas <Divino.Cesar at microsoft.com>
> *Sent:* August 12, 2020 10:41 AM
> *To:* Thomas Stüfe <thomas.stuefe at gmail.com>; David Holmes <
> david.holmes at oracle.com>
> *Cc:* 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
>
> 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://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