RFR (S) 8024517: runtime/CDSCompressedKPtrs/XShareAuto.java failed with RuntimeException

[harold seigel]

Hi,

Please review this updated webrev for bug 8024517: 
http://cr.openjdk.java.net/~hseigel/bug_8024517.2/ 
<http://cr.openjdk.java.net/%7Ehseigel/bug_8024517.2/>

The updates are based on comments from Coleen and David.  CDS will be 
off by default when -server is specified and on by default for -client.  
Also, conditional compilation flags were put around function 
no_shared_spaces() in arguments.cpp to fix a compilation problem when 
building -client.

The ObjectAlignmentInBytes reference was removed from the test along 
with the 64-bit requirement.

Thanks, Harold

On 9/17/2013 3:22 PM, Coleen Phillimore wrote:
>
> Hi David,
>
> On 09/16/2013 11:40 PM, David Holmes wrote:
>> On 17/09/2013 12:53 AM, harold seigel wrote:
>>> Hi David,
>>>
>>> Thanks for the review.
>>
>> Only "attempted review" at that stage :)
>>
>>> My proposed fix maintains the current behavior for the client VM, which
>>> is that CDS is off by default if running on a server class machine.
>>> (Fwiw: the test for server class machine 
>>> (os::is_server_class_machine())
>>> usually returns FALSE for c1 because NeverActAsServerClassMachine
>>> defaults to TRUE for c1 for arm, x86, ppc, and Sparc.)
>>>
>>> The fix only changes the behavior for -server.  Previously, CDS was off
>>> by default for -server only if the JVM was running on a server class
>>> machine.  With this change, CDS will always be off by default when
>>> -server is specified.  The fix implements this change by no longer
>>> checking for a  server class machine when running the server VM.
>>
>> Okay I think I have it now. This all seems rather convoluted - not 
>> sure why we care about server-class-machine to make a CDS choice?
>
> We don't care about server class machine.  We want to turn CDS off by 
> default for -server because it degrades performance.  This 
> server-class-machine means -server almost all of the time but in this 
> corner case didn't mean -server.   So the gist of Harold's change is 
> to turn it off for -server, which we want.
>
> Looking at Harold's change, we should just turn off CDS by default 
> (ie. unless specified) under #ifdef COMPILER2 and not check for server 
> class machine for the client compiler.
>
>>
>>
>> That aside what do we document about the CDS behaviour? Does this 
>> change need CCC request and/or updates to docs?
>
> I think it's not an interesting corner case and not really intended as 
> a change in behavior.  It's what we assumed it was doing in the first 
> place.
>
> I can't comment on the test right now.
>
> Coleen
>>
>>> The test failure reported in the bug occurred because the test was run
>>> with -server on a non server class machine and CDS was not disabled as
>>> expected.
>>
>> Ok. Not sure why the test is 64-bit only though and why does it refer 
>> to ObjectAlignmentInBytes? Is this a copy/n/paste error when the test 
>> was created? I see this under the "compressed Kptrs" which is 
>> presumably 64-bit only, but the tests doesn't see 64-bit related ??
>>
>> Thanks,
>> David
>>
>>> Thanks, Harold
>>>
>>> On 9/14/2013 3:41 AM, David Holmes wrote:
>>>> Hi Harold,
>>>>
>>>> Sorry I am a bit confused about the problem and solution. CDS should
>>>> be off when using the server VM - right? But this code:
>>>>
>>>> ! #ifndef COMPILER2
>>>> !   if (is_server_class_mach)
>>>> ! #endif
>>>>
>>>> only affects the client VM, so you seem to be saying that CDS should
>>>> be off on a client VM when running on a server class machine ???
>>>>
>>>> David
>>>>
>>>> On 14/09/2013 5:05 AM, harold seigel wrote:
>>>>> Hi,
>>>>>
>>>>> Please review this small fix for bug 8024517.  This test failed 
>>>>> when run
>>>>> on a small system because CDS was not disabled by default. 
>>>>> Although the
>>>>> test was run with '-server', CDS was not disabled because function
>>>>> os::is_server_class_machine() returned FALSE.  This fix disables 
>>>>> CDS by
>>>>> default if either os::is_server_class_machine() returns TRUE or the
>>>>> server compiler is running.
>>>>>
>>>>> Also, the '-server' option was added to the test's '-Xshare:dump'
>>>>> command to ensure that the CDS region gets created by the same JVM 
>>>>> that
>>>>> tries to read it.
>>>>>
>>>>> The change was tested with the XShareAuto.java test, by hand using 
>>>>> the
>>>>> NeverActAsServerClassMachine and AlwaysActAsServerClassMachine 
>>>>> options,
>>>>> the UTE vm quicktests, and JPRT.
>>>>>
>>>>> Open webrev at: http://cr.openjdk.java.net/~hseigel/bug_8024517/
>>>>> <http://cr.openjdk.java.net/%7Ehseigel/bug_8024517/>
>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8024517
>>>>>
>>>>> Thanks! Harold
>>>
>