Request for code review - JDK-8141135,Remove G1RemSet::write_ref
david.lindholm at oracle.com
Tue Nov 10 14:48:16 UTC 2015
On 2015-11-10 10:04, Thomas Schatzl wrote:
> 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:
>>> Proposed change:
>> 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
There were an assert(_from != NULL) at the only call to this function.
>> 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