Bug 213686 - ConservativeRoots should mark any cell it finds an interior pointer to
Summary: ConservativeRoots should mark any cell it finds an interior pointer to
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Keith Miller
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2020-06-27 16:21 PDT by Keith Miller
Modified: 2020-06-29 12:08 PDT (History)
7 users (show)

See Also:


Attachments
Patch (9.14 KB, patch)
2020-06-27 16:48 PDT, Keith Miller
no flags Details | Formatted Diff | Diff
Patch for landing (9.26 KB, patch)
2020-06-29 09:24 PDT, Keith Miller
no flags Details | Formatted Diff | Diff
Patch for landing (9.25 KB, patch)
2020-06-29 09:44 PDT, Keith Miller
no flags Details | Formatted Diff | Diff
Patch for landing (9.57 KB, patch)
2020-06-29 11:01 PDT, Keith Miller
no flags Details | Formatted Diff | Diff
Patch for landing (9.58 KB, patch)
2020-06-29 11:06 PDT, Keith Miller
no flags Details | Formatted Diff | Diff
Patch for landing (9.58 KB, patch)
2020-06-29 11:10 PDT, Keith Miller
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Keith Miller 2020-06-27 16:21:52 PDT
ConservativeRoots should any cell it finds an interior pointer to
Comment 1 Keith Miller 2020-06-27 16:48:43 PDT
Created attachment 402975 [details]
Patch
Comment 2 Keith Miller 2020-06-27 18:34:54 PDT
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.
Comment 3 Saam Barati 2020-06-28 19:08:19 PDT
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?
Comment 4 Mark Lam 2020-06-28 19:40:09 PDT
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.
Comment 5 Keith Miller 2020-06-29 09:14:35 PDT
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
Comment 6 Yusuke Suzuki 2020-06-29 09:17:10 PDT
Comment on attachment 402975 [details]
Patch

r=me
Comment 7 Keith Miller 2020-06-29 09:17:37 PDT
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.
Comment 8 Keith Miller 2020-06-29 09:24:11 PDT
Created attachment 403070 [details]
Patch for landing
Comment 9 Keith Miller 2020-06-29 09:44:05 PDT
Created attachment 403074 [details]
Patch for landing
Comment 10 Saam Barati 2020-06-29 10:20:34 PDT
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?
Comment 11 Saam Barati 2020-06-29 10:24:43 PDT
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
Comment 12 Keith Miller 2020-06-29 10:28:29 PDT
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.
Comment 13 Keith Miller 2020-06-29 11:01:55 PDT
Created attachment 403085 [details]
Patch for landing
Comment 14 Keith Miller 2020-06-29 11:06:30 PDT
Created attachment 403087 [details]
Patch for landing
Comment 15 Keith Miller 2020-06-29 11:10:58 PDT
Created attachment 403089 [details]
Patch for landing
Comment 16 EWS 2020-06-29 12:07:30 PDT
Committed r263674: <https://trac.webkit.org/changeset/263674>

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