Request for code review - JDK-8141135,Remove G1RemSet::write_ref

[Thomas Schatzl]

Hi,

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.

Thanks,
  Thomas