Bug 213686 – ConservativeRoots should mark any cell it finds an interior pointer to

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Keith Miller]

ConservativeRoots should any cell it finds an interior pointer to

[Keith Miller]

Created attachment 402975 [details]
Patch

[Keith Miller]

Comment on attachment 402975 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=402975&action=review

> Source/JavaScriptCore/ChangeLog:3
> +        ConservativeRoots should any cell it finds an interior pointer to

Will update the title before committing.

[Saam Barati]

Comment on attachment 402975 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=402975&action=review

> Source/JavaScriptCore/ChangeLog:23
> +        A naive patch that doesn't exit return from
> +        findGCObjectPointersForMarking after finding a live non-butterfly
> +        cell was a 2% regression. So, this patch immediately returns
> +        immediately after marking some non-butterfly. Given this was such
> +        a big regression (likely from running MarkedBlock::isLive) more
> +        than once there's possibly an optimization opportunity here. I
> +        filed https://bugs.webkit.org/show_bug.cgi?id=213687 to
> +        investigate further.

some grammatical issues  in this paragraph.

- 2% regression on what?
- What's "exit return"?
- "immediately returns  immediately after marking some non-butterfly". What do you  mean here? What's the  thing we're doing to make up  perf?

[Mark Lam]

Comment on attachment 402975 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=402975&action=review

Some drive by comments ...

> Source/JavaScriptCore/ChangeLog:16
> +        A naive patch that doesn't exit return from

typo: "exit return".

> Source/JavaScriptCore/ChangeLog:19
> +        cell was a 2% regression. So, this patch immediately returns
> +        immediately after marking some non-butterfly. Given this was such

"immediately" used twice.  I suggest removing the one that preceded "returns".

Also, you didn't explicitly state whether this change fixed the 2% regression, or are we choosing to live with this regression?  Please add a statement to make it clear and explicit what the end result is.

[Keith Miller]

Comment on attachment 402975 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=402975&action=review

>> Source/JavaScriptCore/ChangeLog:23
>> +        investigate further.
> 
> some grammatical issues  in this paragraph.
> 
> - 2% regression on what?
> - What's "exit return"?
> - "immediately returns  immediately after marking some non-butterfly". What do you  mean here? What's the  thing we're doing to make up  perf?

Yeah, regression was fixed in local testing. Will update comments

[Yusuke Suzuki]

Comment on attachment 402975 [details]
Patch

r=me

[Keith Miller]

Comment on attachment 402975 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=402975&action=review

>>> Source/JavaScriptCore/ChangeLog:23
>>> +        investigate further.
>> 
>> some grammatical issues  in this paragraph.
>> 
>> - 2% regression on what?
>> - What's "exit return"?
>> - "immediately returns  immediately after marking some non-butterfly". What do you  mean here? What's the  thing we're doing to make up  perf?
> 
> Yeah, regression was fixed in local testing. Will update comments

The regression was fixed by not checking for interior pointers if we found a pointer to the base of a live object. I would have done this for all pointers including JSImmutableButterfly and Auxiliaries but I don't think that's correct. Since you could have a property only butterfly pointed to off the end of the object.

[Keith Miller]

Created attachment 403070 [details]
Patch for landing

[Keith Miller]

Created attachment 403074 [details]
Patch for landing

[Saam Barati]

Comment on attachment 403074 [details]
Patch for landing

View in context: https://bugs.webkit.org/attachment.cgi?id=403074&action=review

> Source/JavaScriptCore/heap/HeapUtil.h:112
> +            return isLive && !mayHaveIndexingHeader(cellKind);

If we call  "func" because isLive is true, why do we care about  mayHaveIndexingHeader after that?

[Saam Barati]

Comment on attachment 403074 [details]
Patch for landing

View in context: https://bugs.webkit.org/attachment.cgi?id=403074&action=review

>> Source/JavaScriptCore/heap/HeapUtil.h:112
>> +            return isLive && !mayHaveIndexingHeader(cellKind);
> 
> If we call  "func" because isLive is true, why do we care about  mayHaveIndexingHeader after that?

I see why now, because of the one cell over check. Might be worth a comment I guess since it's a bit confusing

[Keith Miller]

Comment on attachment 403074 [details]
Patch for landing

View in context: https://bugs.webkit.org/attachment.cgi?id=403074&action=review

>>> Source/JavaScriptCore/heap/HeapUtil.h:112
>>> +            return isLive && !mayHaveIndexingHeader(cellKind);
>> 
>> If we call  "func" because isLive is true, why do we care about  mayHaveIndexingHeader after that?
> 
> I see why now, because of the one cell over check. Might be worth a comment I guess since it's a bit confusing

Correct, I actually think it's currently only possibly for true butterflies. If we ever add CoW object literals we may have this problem for immutable butterflies too, however, so I just went with the conservative answer. We could also avoid checking for self interior pointers in the case we marked the first value but I think that's rare enough it's not worth the complexity.

Yeah, it seems somewhat non-obvious and no tests break if you always return for butterflies. I'll add a comment so we don't accidentally break it in the future.

[Keith Miller]

Created attachment 403085 [details]
Patch for landing

[Keith Miller]

Created attachment 403087 [details]
Patch for landing

[Keith Miller]

Created attachment 403089 [details]
Patch for landing

[EWS]

Committed r263674: <https://trac.webkit.org/changeset/263674>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 403089 [details].