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

[Alexander Harlap]

Here is change merged with David's change for JDK-8139867:

http://cr.openjdk.java.net/~aharlap/8141135/webrev.01 
<http://cr.openjdk.java.net/%7Eaharlap/8141135/webrev.01>

Now it satisfies to Kim's comments ( no more wrongful / confusing 
assumptions that "from" inside of par_write_ref might be NULL )

Alex



On 11/10/2015 9:48 AM, 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.
>
>
> 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
>>
>>
>>
>