review request (M) - 8030646: track cset membership in one place

[Thomas Schatzl]

Hi,

On Tue, 2013-12-17 at 22:00 -0800, John Coomes wrote:
> Please review a set of changes to eliminate redundant cset membership
> tracking in g1:
> 
> http://cr.openjdk.java.net/~jcoomes/8030646-cset-unify-all.html
> 

8030647-cset-rename-fast-test

While I think renaming is good, I really do not like the new choices.
The base name of the array is now called "_cset_array", which contains
no information about the contents. At least to me the new name looses
information about the purpose of this variable - maybe use
_in_cset_bitmap or so? Or just _in_cset_array would be fine too.

Also I would prefer to add an "is_" prefix to the in-cset check method,
e.g. is_in_cset() instead of in_cset(). This would align with the many
other similar functions in the code.

Maybe even write out "cset" to "collection_set" in the names too; the
existing text in the changed assertions could be improved too.

Also, I do not like the name "clear_cset()" for the method that clears
the _cset_array_raw array. There may be confusion in what this method
does compared to free_collection_set() - it does not clear the
collection set at all. One suggestion could be clear_cset_bitmap (and
possibly name the whole thing cset_bitmap).

8030648-cset-move-methods

Looks good.

8030649-cset-oop-in-cset

I like the introduction of the oop_in_cset() method, but I would prefer
some component in the name that indicates that oop_in_cset does not do
NULL checks.
We have multiple variants in the code (XXX_fast(), XXX_raw(),
XXX_nut_null()) that I like much better than the "oop_" prefix.

I think XXX_raw() is used most in G1 code, but probably XXX_not_null()
is best as this suffix is frequently used with oops.

8030650-cset-rm-length
8030651-cset-unify-obj-in-cs

Look good.

Thanks for looking into this,
  Thomas