RFR: 8059066: CardTableModRefBS might commit the same page twice

[Jon Masamitsu]

On 11/24/2014 08:09 AM, Erik Helin wrote:
> On 2014-11-20, Kim Barrett wrote:
>> On Nov 20, 2014, at 1:27 PM, Kim Barrett <kim.barrett at oracle.com> wrote:
>>> On Nov 17, 2014, at 12:43 PM, Erik Helin <erik.helin at oracle.com> wrote:
>>>> Because the start is aligned down and the end is aligned up, the two
>>>> committed regions will always share at least one page (since the start
>>>> for _committed[1] will be aligned down and the end for _committed[0]
>>>> will be aligned up).
>>> I’m just getting back to this today, and haven’t yet refreshed my understanding
>>> of the code.  However, the claim that there will be at least one shared page,
>>> (with followup reasoning for exactly one shared page), doesn’t seem right to me.
>>> It seems to me there could be zero shared pages, since _committed[1] could
>>> (perhaps accidentally, if not by design) be page aligned.
>> Hm, and it seems that committed regions are always page aligned.  I’m clearly not
>> understanding something yet.  Back to reading code…
> Me and Kim discussed this offline and we both now agree that the existing
> patch does work, but the check if the first _committed region expands into
> the second _committed region can be simpler. The existing check in the
> patch is:
>
>    if (new_end_aligned > _committed[ri].start() &&
>        new_end_aligned <= _committed[ri].end()) {
>
> The check if new_end_aligned <= _committed[ri].end() is not necessary
> because this must always be the case. This is because _committed[0] can't
> expand further than the original, unaligned start of _committed[1] and
> _committed[1] will always commit at least one page in the byte array for
> the card table (starting at _committed[ri].start()). Therefore, it is
> enough to check:
>
>    if (new_end_aligned > _committed[ri].start()) {
>
> I updated the patch to use this simple check and also added an assert to
> show that new_end_aligned <= _committed[ri].end() always is true. Please
> see new webrevs at:
> - full: http://cr.openjdk.java.net/~ehelin/8059066/webrev.01/index.html
> - inc:  http://cr.openjdk.java.net/~ehelin/8059066/webrev.00-01/index.html

Changes look good.

Reviewed.

Jon

>
> Thanks,
> Erik