RFR: 8059066: CardTableModRefBS might commit the same page twice

[Erik Helin]

On 2014-10-27, Kim Barrett wrote:
> On Oct 24, 2014, at 8:54 AM, Erik Helin <erik.helin at oracle.com> wrote:
> >
> > 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?
>
> ...
>
> I have no idea what that's about yet.  But it looks like, for non-G1
> generational collectors the value will actually be 4 rather than 2.  I
> haven't actually verified that with executing code though.
>

The code only ever used two covering (and therefore only two committed)
regions, see the discussion in the review for JDK-8064721 for more
details [0]. The question is now, how many pages can these two committed
regions share?

By reading CardTableModRefBS::find_covering_region_by_base one can see
that when adding a new committed region, the start for the region gets
aligned down:

211   jbyte* ct_start = byte_for(base);
212   uintptr_t ct_start_aligned =
        align_size_down((uintptr_t)ct_start, _page_size);
213   _committed[res].set_start((HeapWord*)ct_start_aligned);

Then CardTableModRefBS::resize_covered_region always aligns up
the end of a region the nearest page:

271     jbyte* const new_end = byte_after(new_region.last());
272     HeapWord* new_end_aligned =
273       (HeapWord*) align_size_up((uintptr_t)new_end, _page_size);

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).

The first committed region can never expand beyond the original,
unaligned start of the second region, because a heap generation cannot
expand into the next heap generation. In other words, the committed
region at index 0 can never expand beyond the page it shares with the
committed region at index 1. This means that the committed regions (when
fully expanded) always will share exactly one page.

The patch in the webrev [1] therefore handles all cases, since it will
correctly determine when _committed[0] extends into the page that is
shared with _committed[1]. The existing code will then adjust the end of
_committed[0] to the start of _committed[1] in order to not commit the
same page twice.

I uploaded a new patch that adds a test (I did not change the fix).
See new webrev at:
http://cr.openjdk.java.net/~ehelin/8059066/webrev.01/

The code in CardTableModRefsBS is very generic, it does not utilize the
fact that we only have two generations, two committed regions and only
one shared page. This makes the code unnecessarily hard to follow. I
filed a new issue to remove these, no longer necessary, generalizations
[2].

[0]: http://mail.openjdk.java.net/pipermail/hotspot-gc-dev/2014-November/011279.html
[1]: http://cr.openjdk.java.net/~ehelin/8059066/webrev.00/
[2]: https://bugs.openjdk.java.net/browse/JDK-8065137