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