RFR: 8069367: assert(_nextMarkBitMap->isMarked((HeapWord*) obj)) failed

[Thomas Schatzl]

Hi Kim,

On Sat, 2015-04-11 at 01:51 -0400, Kim Barrett wrote:
> Another round.
> 
> I've updated my local repo from the team repo, merging with the
> changes from 8076265: Simplify deal_with_reference.
> 
> I've backed out the G1EagerReclaimHumongousPreSnapshotTypeArrays
> experimental configuration option, per Bengt's suggestion that it will
> likely be more trouble than it's worth.
> 
> Because of the latter, the test to determine whether a humongous
> region should be a candidate for eager reclamation has been
> simplified, since at present there is no need to check the allocation
> time of the object.  But I've left the commentary there about
> different handling of objects containing references, and noting the
> special handling of typeArray objects in this respect.
> 
> Also had to move some new inline function definitions from
> g1CollectedHeap.hpp to g1CollectedHeap.inline.hpp, to account for
> header cleanups since my repo was last updated.

Some comments:

- for whatever reason the unified diff is broken, only containing the
TEST.groups and the new test changes.

- I would have preferred the refactoring to be separate from the actual
change.

- why is the test excluded from the hotspot_gc target?

g1CollectedHeap.cpp:

- please avoid reformatting indentation of parameters to something
non-standard (e.g. in G1CollectedHeap::initialize()). The agreed style
is to either have all parameters in the same line as the method call, or
separate parameters lined up one by one below another.

- line 3441: I think it should read "... Also, we must not reclaim an
object that is *on* the mark stack"

- I think the third paragraph is somewhat incomplete: the issue is not
only the remaining remembered set entries, but that objects referenced
from that object would need to be kept alive as well.

- line 3474: I would prefer if the if does not start with the negation.
While this case is arguably the more common one, there does not seem to
be any performance concern at this point. Reading negations is always
harder than the regular form.

Instead of the "if" I recommend calling clear() at collection start for
these reasons:
    - it also resets the humongous candidate value for non-humongous
regions, potentially causing confusion when debugging. I know that the
comment in the code states that this value is ignored for these regions
("Only valid for humongous start regions; other regions have unspecified
values"), but still it will cause you to think about the validity of
this value when seeing it for a non-humongous region.
Also, this results in a defined state for all regions at collection
start, avoiding potential problems.
    - it removes the awkward if-statement mentioned above, decreasing
code complexity.
    - the comment in g1CollectedHeap.hpp " Initialized at start of
collection pause, with candidates removed as they are found reachable
from roots or t..." is not clear about what is initialized to what value
at collection start.
When reading this the first time I would have expected that to mean that
the entire table is initialized to a single default value at collection
start like is generally done, not that "we iterate over all regions to
calculate an initial candidate state value only for these humongous
region starts".
Again, it is fine to define initialization to that (i.e. anything
arbitrarily complex), but this seems to be unnecessarily complicated
compared to just "everything is set to non-candidate".

If this code is kept though (I do not insist on that), I think then the
clear() method could possibly be moved to a protected section or just
removed. (I cannot check easily because the diff is broken).

Calculation of the hrm_index() could be factored out for both cases.

- we try to avoid using two spaces after a full stop in comments.

Thanks,
  Thomas