RFR: 8244801 - Code duplication in ReservedSpace::initialize/ReservedHeapSpace::try_reserve_heap
Cesar Soares Lucas
Divino.Cesar at microsoft.com
Mon Aug 17 18:46:54 UTC 2020
David, no problem. Thanks for reviewing the initial patch.
Thomas, thank you for the review and the detailed explanation.
1-2) I can compile the patch locally with `make images`, but I had only tested it with tier1_part1, testing with the whole tier1 I see the two failed tests that you mention 🙁. I'll later send an updated patch following your suggestions.
3) Thanks for clarifying this. I'll wait to hear from the GC folks to confirm what must be done here and then send an updated patch.
Thx,
Cesar
________________________________
From: hotspot-gc-dev <hotspot-gc-dev-retn at openjdk.java.net> on behalf of David Holmes <david.holmes at oracle.com>
Sent: August 16, 2020 7:07 PM
To: Thomas Stüfe <thomas.stuefe at gmail.com>; Cesar Soares Lucas <Divino.Cesar at microsoft.com>
Cc: hotspot-dev developers <hotspot-dev at openjdk.java.net>; Brian Stafford <Brian.Stafford at microsoft.com>; Aditya Mandaleeka <adityam at microsoft.com>; Hotspot-Gc-Dev <hotspot-gc-dev at openjdk.java.net>
Subject: Re: RFR: 8244801 - Code duplication in ReservedSpace::initialize/ReservedHeapSpace::try_reserve_heap
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://nam06.safelinks.protection.outlook.com/?url=https:%2F%2Fcr.openjdk.java.net%2F~adityam%2Fcesar%2F8244801%2F2%2Fsrc%2Fhotspot%2Fshare%2Fgc%2Fshared%2FgcArguments.cpp.udiff.html&data=02%7C01%7CDivino.Cesar%40microsoft.com%7C20b43e78f0164c9995ec08d84252a4ce%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637332270013934920&sdata=ReQNkLfbqK8e5HWFGpo198oc4zQOE8%2Byyn3Prmgff68%3D&reserved=0
>
> 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://nam06.safelinks.protection.outlook.com/?url=https:%2F%2Fcr.openjdk.java.net%2F~adityam%2Fcesar%2F8244801%2F2%2F&data=02%7C01%7CDivino.Cesar%40microsoft.com%7C20b43e78f0164c9995ec08d84252a4ce%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637332270013944916&sdata=UZ5z6014pawby%2BQEehY%2FkJv4V4PD3X3O64YIEHsb94A%3D&reserved=0
>
> - 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%7C20b43e78f0164c9995ec08d84252a4ce%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637332270013944916&sdata=RQD6huZq0oUnUYGJdje4TqrIYIPOS4%2BJg5TLeRGrrBA%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%7C20b43e78f0164c9995ec08d84252a4ce%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637332270013944916&sdata=RQD6huZq0oUnUYGJdje4TqrIYIPOS4%2BJg5TLeRGrrBA%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%7C20b43e78f0164c9995ec08d84252a4ce%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637332270013944916&sdata=RQD6huZq0oUnUYGJdje4TqrIYIPOS4%2BJg5TLeRGrrBA%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%7C20b43e78f0164c9995ec08d84252a4ce%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637332270013944916&sdata=gkvEJSiOIqrUL22EsDew2yoYsFPPq%2FDC9iD7V4yLgdw%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%7C20b43e78f0164c9995ec08d84252a4ce%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637332270013954909&sdata=OlpCqCGPd4fTGJnyn%2BfxAmWByfRuS0xI9Dd2DsmbQBc%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%7C20b43e78f0164c9995ec08d84252a4ce%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637332270013954909&sdata=OlpCqCGPd4fTGJnyn%2BfxAmWByfRuS0xI9Dd2DsmbQBc%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