RFR: 8062063: Usage of UseHugeTLBFS, UseLargePagesInMetaspace and huge SurvivorAlignmentInBytes cause crashes in CMBitMapClosure::do_bit

[Stefan Johansson]

Hi Jon,

On 2015-01-09 17:48, Jon Masamitsu wrote:
> http://cr.openjdk.java.net/~sjohanss/8062063/hotspot.02/src/share/vm/gc_implementation/g1/g1PageBasedVirtualSpace.cpp.frames.html
>
> Can this be put under the *else *branch for the *if (_special)*
>   147   if (AlwaysPreTouch) {
>   148     os::pretouch_memory(page_start(start), page_start(end));
>   149   }
No, it needs to be outside since the first time we commit "special" 
memory we need to pretouch it the same way as "normal" memory.
> The BitMap
>
>    52   // Bitmap used to keep track of which pages are dirty or not for _special
>    53   // spaces. This is needed because for those spaces the underlying memory
>    54   // will only be zero filled the first time it is committed. Calls to commit
>    55   // will use this bitmap and return whether or not the memory is zero filled.
>    56   BitMap _dirty;
> is used to decide what parts of the memory being brought back (the 
> equivalent of
> the commit() for _special) to zero.  The commit() is passed a start 
> address and size.  What
> is the situation where you need the information in _dirty? Meaning you 
> could zero
> the range passed into commit() i.e.,  (start, start+size), yes? When 
> would that differ
> from using _dirty?
Again the first time is the special case. Since the memory is zero the 
first time we don't what to clear it. If doing this we risk getting 
regressions, and this is the whole reason the zero_filled boolean exist 
for the on_commit method in the G1MappingChangedListener. So basically 
we use the information in _dirty to determine if it is the first time 
memory is committed or not. If it is not the first time commit will 
return the the memory is not zero filled and this is then passed on to 
the on_commit method.

Also, we don't want to zero the range in commit (the reason for doing 
this re-spin), since that would affect all users of 
G1PageBasedVirtualSpace.

Thanks,
Stefan
>
> Jon
> On 1/9/2015 6:31 AM, Stefan Johansson wrote:
>> Updated links:
>> http://cr.openjdk.java.net/~sjohanss/8062063/hotspot.01-02/
>> http://cr.openjdk.java.net/~sjohanss/8062063/hotspot.02/
>>
>> Cheers,
>> Stefan
>>
>> On 2015-01-09 12:31, Stefan Johansson wrote:
>>> Hi Thomas and Kim,
>>>
>>> On 2015-01-08 23:02, Thomas Schatzl wrote:
>>>> Hi Stefan,
>>>>
>>>> On Thu, 2015-01-08 at 14:05 -0500, Kim Barrett wrote:
>>>>> On Jan 8, 2015, at 12:59 PM, Stefan 
>>>>> Johahttp://cr.openjdk.java.net/~sjohanss/8062063/hotspot.01-02/nsson 
>>>>> <stefan.johansson at oracle.com> wrote:
>>>>>> Sorry for doing a re-spin on this, but since this is targeted to 
>>>>>> go into 8 as well I want to minimize the risk of introducing a 
>>>>>> regression.
>>>>>>
>>>>>> After yesterdays comments I started thinking more about what 
>>>>>> regressions this fix might cause and today I've had good 
>>>>>> discussions with Thomas and Mikael. I've also did some quick 
>>>>>> measurements that shows additional time for the YCs expanding the 
>>>>>> heap after a shrink. Since we don't really need the heap regions 
>>>>>> to be cleared I think we need to avoid this regression, by going 
>>>>>> with another solution and I don't think having this time added to 
>>>>>> the full GC shrinking the heap is wanted either.
>>>>>>
>>>>>> The first proposal that is explained in the bug-report would 
>>>>>> avoid clearing memory that don't have to be cleared, but just 
>>>>>> doing the simple solution explained there might cause startup 
>>>>>> regressions due to touching memory during startup that isn't 
>>>>>> needed. Mixing that approach with the one proposed yesterday will 
>>>>>> allow us to only clear memory when absolutely needed. See new 
>>>>>> webrev here:
>>>>>> http://cr.openjdk.java.net/~sjohanss/8062063/hotspot.01
>>>>>>
>>>>>> This approach leaves the clearing to the listener registered with 
>>>>>> each mapper and for the bitmaps this will make sure that they are 
>>>>>> cleared, but for the heap we won't do anything (because the heap 
>>>>>> has no requirement of having zeroed backing memory).
>>>>> I think I might prefer having the new bitmap called _zero_filled and
>>>>> flip the sense of it.  The present name, _needs_zeroing, is mildly
>>>>> confusing to me, since whether zeroing is needed is caller-dependent.
>>>> I somewhat tend to agree with Kim here.
>>>>
>>>> Maybe one of _needs_clear_on_commit, _not_zero_on_commit,
>>>> _is_clear_on_commit, or _pages_dirty_after_uncommit, but ymmv. :)
>>> I too agree, but I changed it to simply _dirty and added a big 
>>> comment for it. This lets me keep the current logic where a 1 in the 
>>> bitmap means it is not filled with zeros (dirty).
>>>
>>> New webrev and incremental one:
>>> http://wikifiles.se.oracle.com/dev/sjohanss/8062063/hotspot.02/
>>> http://wikifiles.se.oracle.com/dev/sjohanss/8062063/hotspot.01-02/
>>>
>>> Thanks,
>>> Stefan
>>>
>>>>> It seems to me this new version can result in unnecessary page
>>>>> clearing; commit will return true if any page in the range is not
>>>>> zeroed.  This can lead to a caller that needs zeroed pages clearing
>>>>> the entire requested range, even if only some (perhaps small) subset
>>>>> of the range is actually dirty.
>>>>>
>>>>> Of course, the previous attempted fix also had unnecessary page
>>>>> clearing, since dirty pages were being cleared even if the caller
>>>>> doesn't care.  The new code seems likely to be an improvement 
>>>>> overall.
>>>>>
>>>>> In the context of fixing the bug at hand, I think this change looks
>>>>> good, up to the naming and sense of the new bit map.
>>>>>
>>>>> But it looks like the API provided by G1PageBasedVirtualSpace is less
>>>>> than ideal in this area, and could perhaps use further work. Though
>>>>> it might not be worth worrying about, as the cases where it matters
>>>>> may be rare and not especially important.
>>>> It simply assumes that a commit() zero-fills the page lazily.
>>>>
>>>> I do not think it is worth worrying a lot about it. There need to be a
>>>> lot of circumstances involved, and the new change at least always 
>>>> avoids the
>>>> clearing of the Java heap space.
>>>>
>>>> The best solution would simply be doing away with the pre-commit hack
>>>> when using large pages :)
>>>>
>>>>    Thomas
>>>>
>>>>
>>>>
>>>
>>
>

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