RFR(S) JDK-8025526: VirtualSpace should support per-instance disabling of large pages

[Mikael Gerdin]

Thomas,

On 09/30/2013 03:57 PM, Thomas Schatzl wrote:
> Hi Mikael,
>
> On Mon, 2013-09-30 at 12:49 +0200, Mikael Gerdin wrote:
>> Erik,
>>
>> On 09/27/2013 05:10 PM, Erik Helin wrote:
>>> Hi Mikael,
>>>
>>> what do you think about splitting the VirtualSpace::initialize method
>>> into two methods:
>>> - VirtualSpace::initialize
>>> - VirtualSpace::initialize_with_granularity
>>>
>>> Then initialize would just call initialize_with_granularity like the
>>> following:
>>>
>>>     bool VirtualSpace::initialize(ReservedSpace rs, size_t committed_size) {
>>>       size_t granularity = os::page_size_for_regions(rs.size(), rs.size(), 1);
>>>       return initialize_with_granularity(rs, committed_size, granularity);
>>>     }
>>>
>>>     bool VirtualSpace::initialize_with_granularity(ReservedSpace rs,
>>>       size_t committed_size, size_t granularity) {
>>>       assert(granularity > 0, "Can't have 0 as granularity");
>>>       // old initialize code
>>>       _middle_alignment = granularity;
>>>       // rest of old initialized code
>>>     }
>>>
>>> This way, we get rid of the default argument and don't have to check if
>>> max_commit_granularity == 0.
>>
>> I like your suggestion.
>> I've uploaded a second webrev which implements your suggestion.
>
> Could the patch changed to remove the default parameter specification
> for the max_commit_granularity parameter in
> initialize_with_granularity()?
>
> The code basically checks that the given granularity is >0, so a default
> value of 0 invites errors.

I forgot to remove the default value. I'll fix that in the next webrev.

>
>> I had to update the test code so I re-ran the tests on all platforms.
>>
>> New webrev: http://cr.openjdk.java.net/~mgerdin/8025526/webrev.1/
>
> Looks good otherwise.

Thanks.

/Mikael

>
> Thanks,
>    Thomas
>
>