Request for code review - JDK-8141135,Remove G1RemSet::write_ref
alexander.harlap at oracle.com
Wed Nov 11 16:30:27 UTC 2015
Here is change merged with David's change for JDK-8139867:
Now it satisfies to Kim's comments ( no more wrongful / confusing
assumptions that "from" inside of par_write_ref might be NULL )
On 11/10/2015 9:48 AM, David Lindholm wrote:
> 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 in from");
> 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