Request for code review - JDK-8141135,Remove G1RemSet::write_ref
thomas.schatzl at oracle.com
Tue Nov 10 09:04:07 UTC 2015
On Mon, 2015-11-09 at 23:57 -0500, Kim Barrett wrote:
> On Nov 9, 2015, at 12:12 PM, Alexander Harlap <alexander.harlap at oracle.com> wrote:
> > Remove some unused code:
> > JDK-8141135
> > Proposed change:
> > http://cr.openjdk.java.net/~aharlap/8141135/webrev.00
> > Alex
> The description of write_ref / par_write_ref doesn't entirely match
> the implementation.
> The description says "from" must be non-NULL. However, the assertion
> validating "from" and that "p" is within it explicitly checks for and
> allows "from" == NULL.
That assertion will crash if "from" is NULL. Not sure if crashes
indicate being fine to use it.
Also one could interpret the ", which is required to be non-NULL" could
refer to "p" as well, which is the subject that is talked about. But
that is just my interpretation of the sentence... (but I can be
convinced that it can be read your way).
I think it is only implied that "from" must be non-NULL, since "p" must
be non-NULL. If p is NULL, the method will also immediately crash.
Maybe move the assert that checks from->is_in_reserved(p) to the top of
the method body and add a clause that checks from != NULL?
> There appears to only be one call site, in
> UpdateRSOopClosure::do_oop_work. That call site is preceeded by an
> assertion that the region value that will be passed to par_write_ref
> is non-NULL. So I think the allowance of NULL "from" in par_write_ref
> is unnecessary, and contrary to its description.
More information about the hotspot-gc-dev