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

Thomas Schatzl thomas.schatzl at oracle.com
Tue Nov 10 15:22:36 UTC 2015


Hi,

On Tue, 2015-11-10 at 15:48 +0100, David Lindholm wrote:
> 
> On 2015-11-10 10:04, Thomas Schatzl wrote:
> > 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?
> 
> This assert does not look like that any more. It now looks like this:
> 
> assert(from->is_in_reserved(p) || from->is_starts_humongous(), "p is not 
> in from");
> 
> There were an assert(_from != NULL) at the only call to this function.

  actually I have already been looking on the current code and assert,
that is different to Alex' baseline.

This is probably where the confusion comes from.

Thanks,
  Thomas




More information about the hotspot-gc-dev mailing list