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

[Stefan Karlsson]

Hi Thomas,

On 08/28/2013 03:33 PM, Thomas Schatzl wrote:
> Hi,
>
>    can I have reviews for an update of this change? It is now based on CR
> 8007074, Stefan Karlsson's large pages patch. This required minor
> changes.
>
> All other issues mentioned below have been addressed as well I think.
>
> bugs.sun.com
> http://bugs.sun.com/view_bug.do?bug_id=8016825
>
> Webrev:
> http://cr.openjdk.java.net/~tschatzl/8016825/webrev.2

http://cr.openjdk.java.net/~tschatzl/8016825/webrev.2/src/os/windows/vm/os_windows.cpp.udiff.html

+    if (Verbose) {
+      gclog_or_tty->print("Skipping test because large pages are disabled");
+    }

Have you considered using the new  VerboseInternalVMTests flag I'm using 
in other InternalVMTests?

http://cr.openjdk.java.net/~tschatzl/8016825/webrev.2/src/share/vm/prims/jni.cpp.sdiff.html

5044 // Forward declaration
5045 void TestReserveMemorySpecial_test();
5046
5047 #define run_unit_test(unit_test_function_call)              \
5048   tty->print_cr("Running test: " #unit_test_function_call); \
5049   unit_test_function_call
5050
5051 // Forward declaration
5052 void TestReservedSpace_test();
5053 void TestReserveMemorySpecial_test();

Are lines 5044-5046 some merge artifact. We already have the forward declaration on line 5053.

Otherwise, this looks good to me.

thanks,
StefanK


>
> Testing:
> New VM internal test case, jprt
>
> Thanks,
> Thomas
>
> On Wed, 2013-07-17 at 09:50 +0200, Thomas Schatzl wrote:
>> Hi,
>>
>> On Tue, 2013-07-16 at 14:14 +0200, Bengt Rutisson wrote:
>>> 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
>> You are correct about the rounding. I expected it to not work as the
>> documentation of MEM_LARGE_PAGES indicated that the size and alignment
>> must already be a multiple of large page size.
>>
>> A small test however proved the contrary.
>>
>>> 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 patch makes sure that the requested address is aligned to a
>> multiple of the heap maximum alignment, which is a multiple of the large
>> page size if they are used.
>>
>> See
>> http://cr.openjdk.java.net/~stefank/8007074/webrev.00/src/share/vm/memory/universe.cpp.udiff.html
>>
>> the preferred_heap_base_address() function.
>>
>> However it is certainly nice to check the alignment again (Stefan's
>> patch does again in os::reserve_memory_special() - but only for Linux. I
>> will add some for Windows).
>>
>>> Stefan's large page changes
>>> enforces much more alignment, so maybe it is worth waiting with this
>>> patch until he has pushed his changes?
>> I somewhat agree because this would just replicate that work.
>>
>>> 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
>>>
>> No, this is a bug in the patch. I am not used to working on Windows, the
>> IDE added tabs, after that I set the IDE to replace tabs with space, but
>> forgot this one when fixing this manually.
>>
>>> 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?
>> Can do, although looking back I do not see the need for introducing an
>> extra flag.
>>   
>>> 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 =
>> I will fix this. Sorry.
>>   
>>> BTW, maybe we can have a better name than result? Maybe base_address?
>> Okay.
>>
>>> 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 tried a similar approach first - it did not seem to work at that time,
>> after deallocating even the exact same address was not available any
>> more.
>>
>> When retrying just now (on a freshly booted machine), it works, so there
>> might have been another issue. I will change that.
>>
>>> 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.
>> The proposal sounds fine, but I am not completely clear what the "more
>> issues" such a test will catch are. Further I am worried about false positives.
>>
>> The main problem is that large page allocation is only a wish to the OS;
>> failure is normal behavior.
>>
>> E.g. on Windows, failure seems to be common after a while: after running
>> it for only ~7 hours, the test fails to allocate the "control block"
>> already (the 2M block with address NULL) - on a 64 bit VM with 8G of
>> RAM. This seems to be a side effect of the requirement that in Windows,
>> large pages need to be always be backed by physical pages, and it seems
>> that memory in windows becomes very fragmented after a while.
>>
>> The only "cure" for this I know of is reboot. I am not sure if it is
>> acceptable to reboot the Windows machine for every test run to avoid
>> lots of false positives. :)
>>
>> There are similar issues on the other OSes.
>>
>> As mentioned, getting large pages is just a request. In some cases large pages may only be reserved over time (e.g. transparent huge pages on Linux to some extent as far as I understand them; I am not sure what Solaris does).
>>
>> So checking for actual pages may not be useful as there is a relatively high chance that it will give frequent false positives; one alternative is to check whether the VM tried to request large pages or not?
>>
>> Another problem seems to be: what is the response of the test if the OS just does not give the VM large pages? The VM already warns if it could not allocate large pages for the heap if possible.
>>
>> Thanks for your comments. I will update the change though with your comments,
>> Thomas
>>
>>
>>
>>
>>
>