Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2673693003:
[wrapper-tracing] Don't call into V8 during heap snapshot creation (Closed)
|
Created:
3 years, 10 months ago by Michael Lippautz Modified:
3 years, 10 months ago Reviewers:
haraken CC:
chromium-reviews, blink-reviews, blink-reviews-bindings_chromium.org, kozy, ulan Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[wrapper-tracing] Don't call into V8 during heap snapshot creation
Without this fix we would call into V8 for marking an object when
creating a heap snapshot. The intented behavior is to intercept marking
and record an edge in the snapshot. We cannot do that right now as we
cannot properly intercept marking in GlobalValueMaps and their friends.
Note that this is not a regression as we still record the object as live
in the snapshot and we also didn't record the edge using object
grouping.
BUG=chromium:688162
Review-Url: https://codereview.chromium.org/2673693003
Cr-Commit-Position: refs/heads/master@{#448056}
Committed: https://chromium.googlesource.com/chromium/src/+/c03dcfbe13aecc48b0b4cd0a107a8d659bdda939
Patch Set 1 #
Total comments: 4
Patch Set 2 : Comment #
Total comments: 2
Patch Set 3 : Add DCHECK #Messages
Total messages: 20 (13 generated)
mlippautz@chromium.org changed reviewers: + haraken@chromium.org
ptal
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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2673693003/diff/1/third_party/WebKit/Source/b... File third_party/WebKit/Source/bindings/core/v8/V8GCController.cpp (right): https://codereview.chromium.org/2673693003/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/bindings/core/v8/V8GCController.cpp:199: const ScriptWrappable* traceable) const override { What happens if this is called on a worker thread? Won't it end up with tracing edges on the main thread? https://codereview.chromium.org/2673693003/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/bindings/core/v8/V8GCController.cpp:201: // DOMWrapperMap::markWrapper. Also explain that this means that we won't be able to explain edges from wrappers in isolated worlds.
https://codereview.chromium.org/2673693003/diff/1/third_party/WebKit/Source/b... File third_party/WebKit/Source/bindings/core/v8/V8GCController.cpp (right): https://codereview.chromium.org/2673693003/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/bindings/core/v8/V8GCController.cpp:199: const ScriptWrappable* traceable) const override { On 2017/02/03 03:55:45, haraken wrote: > > What happens if this is called on a worker thread? Won't it end up with tracing > edges on the main thread? Snapshots can only be created from the main thread. https://codereview.chromium.org/2673693003/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/bindings/core/v8/V8GCController.cpp:201: // DOMWrapperMap::markWrapper. On 2017/02/03 03:55:45, haraken wrote: > > Also explain that this means that we won't be able to explain edges from > wrappers in isolated worlds. > Done.
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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM https://codereview.chromium.org/2673693003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/V8GCController.cpp (right): https://codereview.chromium.org/2673693003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8GCController.cpp:138: m_firstScriptWrappableTraced(false) {} Shall we add DCHECK(isMainThread()) (for a documentation purpose)?
https://codereview.chromium.org/2673693003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/V8GCController.cpp (right): https://codereview.chromium.org/2673693003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8GCController.cpp:138: m_firstScriptWrappableTraced(false) {} On 2017/02/03 07:46:12, haraken wrote: > > Shall we add DCHECK(isMainThread()) (for a documentation purpose)? Good idea, done.
The CQ bit was checked by mlippautz@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org Link to the patchset: https://codereview.chromium.org/2673693003/#ps40001 (title: "Add DCHECK")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1486137094926390,
"parent_rev": "58feb04ce1dda2ff33edaf1973de3f4fefb5f56e", "commit_rev":
"c03dcfbe13aecc48b0b4cd0a107a8d659bdda939"}
Message was sent while issue was closed.
Description was changed from ========== [wrapper-tracing] Don't call into V8 during heap snapshot creation Without this fix we would call into V8 for marking an object when creating a heap snapshot. The intented behavior is to intercept marking and record an edge in the snapshot. We cannot do that right now as we cannot properly intercept marking in GlobalValueMaps and their friends. Note that this is not a regression as we still record the object as live in the snapshot and we also didn't record the edge using object grouping. BUG=chromium:688162 ========== to ========== [wrapper-tracing] Don't call into V8 during heap snapshot creation Without this fix we would call into V8 for marking an object when creating a heap snapshot. The intented behavior is to intercept marking and record an edge in the snapshot. We cannot do that right now as we cannot properly intercept marking in GlobalValueMaps and their friends. Note that this is not a regression as we still record the object as live in the snapshot and we also didn't record the edge using object grouping. BUG=chromium:688162 Review-Url: https://codereview.chromium.org/2673693003 Cr-Commit-Position: refs/heads/master@{#448056} Committed: https://chromium.googlesource.com/chromium/src/+/c03dcfbe13aecc48b0b4cd0a107a... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/c03dcfbe13aecc48b0b4cd0a107a... |
