RFR: 8059066: CardTableModRefBS might commit the same page twice

[Erik Helin]

On 2014-10-20 21:52, Kim Barrett wrote:
> On Oct 20, 2014, at 9:21 AM, Erik Helin <erik.helin at oracle.com> wrote:
>>
>> Webrev:
>> http://cr.openjdk.java.net/~ehelin/8059066/webrev.00/
>>
>> Bug:
>> https://bugs.openjdk.java.net/browse/JDK-8059066
>
> ==============================================================================
> src/share/vm/memory/cardTableModRefBS.cpp
> 280       if (new_end_aligned > _committed[ri].start() &&
> 281           new_end_aligned <= _committed[ri].end()) {
>
> Is the test against _committed[ri].end() on line 281 correct?
>
> I *think* the intended criteria for interference is
> !new_region.intersection(_committed[ri]).is_empty()
> with the understanding at line 280 that
> new_region.start() <= _committed[ri].start().
> which simplifies the needed checking.  (There's an assertion to that
> effect in the if-body.)
>
> I think that line 281 will fail to detect the situation where
> _committed[ri] is a proper subset of new_region, and thus the
> intersection is non-empty.
>
> On the other hand, I'm not sure what should be done in that
> situation.  Maybe that situation is just not supposed to arise for
> some reason?  E.g. something in the caller prevents that?  If so, an
> assertion somewhere here would seem appropriate, but I don't see one
> that deals with this case.

You are correct that the check is not enough to cover all cases. 
Regarding the case of proper subsets, there is a comment a few lines 
above in the code stating:

264     // This forms overlapping regions, but never interior regions.

so it seems like the author intended for the card table to *only* have 
overlapping regions, not interior ones (i.e. subsets). The check

   !new_region.intersection(_committed[ri]).is_empty()

could mean that `_comitted[ri]` is a subset of `new_region`. 
Furthermore, the above check is not enough in the case of multiple 
subsets, you might have the case where you have multiple regions 
following `ind`, and `new_region` might be larger than one (or more) of 
them, but still need to commit more pages if there are "holes":

   ... | ind |     | xxx |     | yyy |     |     |
                                           ^
In the case above, ind will to have cover xxx and yyy and also commit 
the page after yyy. In order to not have xxx and yyy become interior 
regions (subsets), their ends would have to be updated to match the end 
`new_region`. The existing code seems to assume that this can can not 
happen:

296         // Should only collide with 1 region

Fortunately for us, we only have have two committed regions (anyone, 
please correct me if I'm wrong here) and two covering regions, since we 
only have two generations. Unfortunately, CardTableModRefBS was probably 
used for the train collector and therefore (unnecessary) tries to 
support n generations.

In the case of at most two committed regions and no subsets, I believe 
that my proposed fix will work (I should have mentioned all this is in 
the original RFR). What do you think? Should we add asserts that checks 
that the number of committed regions are at most two?

On 2014-10-20 21:52, Kim Barrett wrote:
> ==============================================================================
> src/share/vm/memory/cardTableModRefBS.cpp
> There are some later assertions in the same function which look like
> they might have a similar problem, e.g.
>
> 302       assert(!_committed[ri].contains(new_end_aligned),

Agree, should be updated (the assert is not wrong, it is only weaker 
than it could be).

On 2014-10-20 21:52, Kim Barrett wrote:
> 387         // The end of the new committed region should not
> 388         // be in any existing region unless it matches
> 389         // the start of the next region.
> 390         assert(!_committed[ri].contains(end) ||
> 391                (_committed[ri].start() == (HeapWord*) end),
> 392                "Overlapping committed regions");

Agree, same case as the above.

Thanks,
Erik