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

[David Lindholm]

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.


Thanks,
David

>> 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
>
>
>