Summary: | ConservativeRoots should mark any cell it finds an interior pointer to | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Keith Miller <keith_miller> | ||||||||||||||
Component: | New Bugs | Assignee: | Keith Miller <keith_miller> | ||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||
Severity: | Normal | CC: | ews-watchlist, mark.lam, msaboff, saam, tzagallo, webkit-bug-importer, ysuzuki | ||||||||||||||
Priority: | P2 | ||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||
OS: | Unspecified | ||||||||||||||||
Attachments: |
|
Description
Keith Miller
2020-06-27 16:21:52 PDT
Created attachment 402975 [details]
Patch
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 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 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 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 on attachment 402975 [details]
Patch
r=me
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. Created attachment 403070 [details]
Patch for landing
Created attachment 403074 [details]
Patch for landing
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 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 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. Created attachment 403085 [details]
Patch for landing
Created attachment 403087 [details]
Patch for landing
Created attachment 403089 [details]
Patch for landing
Committed r263674: <https://trac.webkit.org/changeset/263674> All reviewed patches have been landed. Closing bug and clearing flags on attachment 403089 [details]. |