RFR (XS) 8016825 Large pages for the heap broken on Windows for compressed oops

[Bengt Rutisson]

Hi Thomas,

Thanks for fixing this.

According to the MSDN documentation for VirtualAlloc the address 
parameter can be rounded down if it is not properly aligned:
http://msdn.microsoft.com/en-us/library/windows/desktop/aa366887%28v=vs.85%29.aspx

Does that mean that we in os::reserve_memory_special() should assert 
that addr is large page aligned? If it is not we will probably get a 
different return address than we asked for anyway. I had a quick look 
through the call chain. I am not sure it is always large page aligned. 
Do we need to align it in ReservedSpace::initialize() or should we just 
assert that it is aligned? Stefan's large page changes enforces much 
more alignment, so maybe it is worth waiting with this patch until he 
has pushed his changes?

Some minor comments:

The webrev looks a bit odd here. Is there a format issue in the patch or 
is it just the webrev?
http://cr.openjdk.java.net/~tschatzl/8016825/webrev/src/os/posix/vm/os_posix.cpp.frames.html

You use Verbose for logging in your test. I think that is fine. Stefan 
recently added a new flag in one of his webrevs called 
VerboseInternalVMTests:
http://cr.openjdk.java.net/~stefank/8007074/webrev.00/src/share/vm/runtime/globals.hpp.udiff.html
That has not been pushed yet, but maybe it is worth introducing this 
flag with your change instead?

Minor format thing in the test code. You have spaces between the type 
and the star. I think we normally don't have that. So for example:

char * result =

should probably be:

char* result =

BTW, maybe we can have a better name than result? Maybe base_address?


I don't understand the allocation sizes. Why do we pick 16 large pages?

Would it be possible to do it like this instead?

* First allocate 3 large pages at any address
* Remember the returned address (base)
* Release the memory
* Allocate 2 large pages at (base address + one large page size)

I think that will reduce the risk of entering this case:

5627     if (actual_location == NULL) {
5628       if (Verbose) {
5629         gclog_or_tty->print("Failed to allocate any memory at 
"PTR_FORMAT" size "SIZE_FORMAT". Skipping remainder of test.",
5630           expected_location, large_allocation_size);
5631       }
5632     }

I like the fact that you added a test, but as you said it does not test 
that much. It does help a bit, so I think we should keep it. But have 
you thought about trying to write a JTreg test that can be run with 
UseLargePages and then check with the OS that large pages are actually 
being used? I think it will be much more work to write such a test, but 
I think it would catch more issues.

Thanks,
Bengt


On 7/15/13 11:33 AM, Thomas Schatzl wrote:
> Hi all,
>
>    can I have reviews for the following change?
>
> The change fixes the allocation of memory using large pages on Windows
> when a particular address is requested under some circumstances.
>
> In particular, VirtualAlloc is always called with a NULL address,
> meaning there is no preferred allocation address, which is not the
> desired behavior.
>
> See src/os/windows/vm/os_windows.cpp line 3155 in the original code.
>
> The other part of the changes contain a test case for that.
>
> This test case is a bit convoluted, as it needs to handle a few edge
> cases which are valid behaviors (mostly not getting large pages in the
> first place) that should not cause a failed test.
>
> There is no check whether the returned memory consists of large pages; the call to VirtualAlloc() with MEM_LARGE_PAGES always returns large page memory on success.
>
> webrev:
> http://cr.openjdk.java.net/~tschatzl/8016825/webrev/
>
> jbs:
> https://jbs.oracle.com/bugs/browse/JDK-8016825
>
> bugs.sun
> http://bugs.sun.com/view_bug.do?bug_id=8016825
>
> testing:
> jprt running the test case, manual testing
>
> To reproduce the test case, you must give yourself permission to use
> large pages (http://msdn.microsoft.com/en-us/library/ms190730.aspx) and
> run the VM in an elevated shell with -XX:+UseLargePages and -XX:
> +ExecuteInternalVMTests .
>
> Thanks,
>    Thomas
>
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20130716/100a697e/attachment.htm>