Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2529353002:
[wrapper-tracing] Avoid GC during PostMessageTimer construction (Closed)
|
Created:
4 years ago by Michael Lippautz Modified:
4 years ago CC:
blink-reviews, chromium-reviews Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[wrapper-tracing] Avoid GC during PostMessageTimer construction
BUG=chromium:668742, chromium:668059, chromium:668060
Committed: https://crrev.com/ca07ee393934e403d0bb2e0d8790d8edbbe3df96
Cr-Commit-Position: refs/heads/master@{#434638}
Patch Set 1 #
Total comments: 2
Messages
Total messages: 25 (13 generated)
Description was changed from ========== [wrapper-tracing] Avoid GC during PostMessageTimer construction BUG= ========== to ========== [wrapper-tracing] Avoid GC during PostMessageTimer construction BUG=chromium:668742 ==========
Description was changed from ========== [wrapper-tracing] Avoid GC during PostMessageTimer construction BUG=chromium:668742 ========== to ========== [wrapper-tracing] Avoid GC during PostMessageTimer construction BUG=chromium:668742,chromium:668059 ==========
Description was changed from ========== [wrapper-tracing] Avoid GC during PostMessageTimer construction BUG=chromium:668742,chromium:668059 ========== to ========== [wrapper-tracing] Avoid GC during PostMessageTimer construction BUG=chromium:668742,chromium:668059, chromium:668060 ==========
mlippautz@chromium.org changed reviewers: + haraken@chromium.org, hlopko@chromium.org
This one should deal with the vast majority of CHECK failures on current Canaries.
On 2016/11/28 10:35:09, Michael Lippautz wrote: > This one should deal with the vast majority of CHECK failures on current > Canaries. fyi: Was also flushed out by a CF issue.
LGTM, that must've been hard to spot!
The CQ bit was checked by mlippautz@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
LGTM That said, we should add a more generic mechanism to avoid triggering an incremental tracing while isGCForbidden is true.
On 2016/11/28 10:56:26, haraken wrote: > LGTM > > That said, we should add a more generic mechanism to avoid triggering an > incremental tracing while isGCForbidden is true. While it would make our lives easier if we could do that, I am not yet convinced we can postpone in all cases. I can see us postponing incremental marking steps and also starting wrapper tracing from incremental marking steps. However, when we finally do a full GC in V8 we cannot delay anymore. I will give this some more thought.
On 2016/11/28 11:48:04, Michael Lippautz wrote: > On 2016/11/28 10:56:26, haraken wrote: > > LGTM > > > > That said, we should add a more generic mechanism to avoid triggering an > > incremental tracing while isGCForbidden is true. > > While it would make our lives easier if we could do that, I am not yet convinced > we can postpone in all cases. > > I can see us postponing incremental marking steps and also starting wrapper > tracing from incremental marking steps. However, when we finally do a full GC in > V8 we cannot delay anymore. > > I will give this some more thought. Would it be possible to delay even a full GC to the next V8 allocation? (Oilpan is doing that.) My concern is that the current programming constraint is a bit too error-prone for Blink developers. There's no easy way to figure out a V8 object allocation during a Blink object construction. Another option might be to add DCHECK(!isGCForbidden()) to the call path of a V8 object allocation so that the developers can notice the programming error more easily (before hitting a CHECK in an undeterministic GC).
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by mlippautz@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/11/28 11:55:17, haraken wrote: > On 2016/11/28 11:48:04, Michael Lippautz wrote: > > On 2016/11/28 10:56:26, haraken wrote: > > > LGTM > > > > > > That said, we should add a more generic mechanism to avoid triggering an > > > incremental tracing while isGCForbidden is true. > > > > While it would make our lives easier if we could do that, I am not yet > convinced > > we can postpone in all cases. > > > > I can see us postponing incremental marking steps and also starting wrapper > > tracing from incremental marking steps. However, when we finally do a full GC > in > > V8 we cannot delay anymore. > > > > I will give this some more thought. > > Would it be possible to delay even a full GC to the next V8 allocation? (Oilpan > is doing that.) > I will check that. I don't think we can always do that. > My concern is that the current programming constraint is a bit too error-prone > for Blink developers. There's no easy way to figure out a V8 object allocation > during a Blink object construction. Totally agree! > > Another option might be to add DCHECK(!isGCForbidden()) to the call path of a V8 > object allocation so that the developers can notice the programming error more > easily (before hitting a CHECK in an undeterministic GC). Yes, I was thinking of a more pragmatic solution that will fail fast. Since almost any path into V8 can lead to allocations I think we could add such a check somewhere on the API boundary.
CQ is committing da patch.
Bot data: {"patchset_id": 1, "attempt_start_ts": 1480334639507550, "parent_rev":
"46a51c1d947d2a5d555f47ce5e4430890af2d454", "commit_rev":
"209548fbf3832b8e740da382562e8519700ef01d"}
Message was sent while issue was closed.
Description was changed from ========== [wrapper-tracing] Avoid GC during PostMessageTimer construction BUG=chromium:668742,chromium:668059, chromium:668060 ========== to ========== [wrapper-tracing] Avoid GC during PostMessageTimer construction BUG=chromium:668742,chromium:668059, chromium:668060 ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1)
Message was sent while issue was closed.
Description was changed from ========== [wrapper-tracing] Avoid GC during PostMessageTimer construction BUG=chromium:668742,chromium:668059, chromium:668060 ========== to ========== [wrapper-tracing] Avoid GC during PostMessageTimer construction BUG=chromium:668742,chromium:668059, chromium:668060 Committed: https://crrev.com/ca07ee393934e403d0bb2e0d8790d8edbbe3df96 Cr-Commit-Position: refs/heads/master@{#434638} ==========
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/ca07ee393934e403d0bb2e0d8790d8edbbe3df96 Cr-Commit-Position: refs/heads/master@{#434638}
Message was sent while issue was closed.
sigbjornf@opera.com changed reviewers: + sigbjornf@opera.com
Message was sent while issue was closed.
https://codereview.chromium.org/2529353002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/frame/LocalDOMWindow.cpp (right): https://codereview.chromium.org/2529353002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/frame/LocalDOMWindow.cpp:628: std::unique_ptr<SourceLocation> location = SourceLocation::capture(source); Subtle staging of allocations going on here wrt 'new'; an explanatory comment covering why the allocation needs to be lifted out, would have been helpful. Even better, following the standard pattern of having a create() factory method would have prevented the problem from arising, i think.
Message was sent while issue was closed.
https://codereview.chromium.org/2529353002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/frame/LocalDOMWindow.cpp (right): https://codereview.chromium.org/2529353002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/frame/LocalDOMWindow.cpp:628: std::unique_ptr<SourceLocation> location = SourceLocation::capture(source); On 2016/11/29 09:15:44, sof wrote: > Subtle staging of allocations going on here wrt 'new'; an explanatory comment > covering why the allocation needs to be lifted out, would have been helpful. > Sorry, for not leaving a comment. The rationale is that we cannot call into V8 from a gcForbidden scope, because V8 can now trace through blink, effectively requiring some of its marking infrastructure. > Even better, following the standard pattern of having a create() factory method > would have prevented the problem from arising, i think. I will add (D)CHECKs that would immediately fail (even without triggering a GC) when calling into V8 from such scopes. Only those call sites require refactoring. Using ::create might mitigate this scenario, but people could still call it from ctors before vtable poitners are available. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
