[profiler] Emit standalone JS to cppgc edges [v8/v8 : main]
0 views
Skip to first unread message
Dominik Inführ (Gerrit)
Mar 25, 2026, 4:37:35 AM (6 days ago) Mar 25
to Michael Lippautz, V8 LUCI CQ, Hannes Payer, devtools-...@chromium.org, mlippau...@chromium.org, oilpan-r...@chromium.org, v8-re...@googlegroups.com
Attention needed from Michael Lippautz
Dominik Inführ voted and added 1 comment![]( "Open in Gerrit")
Votes added by Dominik Inführ
| Commit-Queue | +1 |
1 comment
Patchset-level comments
Related details
Attention is currently required from:
- Michael Lippautz
Submit Requirements:
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michael Lippautz (Gerrit)
Mar 25, 2026, 4:49:56 AM (6 days ago) Mar 25
to Dominik Inführ, V8 LUCI CQ, Hannes Payer, devtools-...@chromium.org, mlippau...@chromium.org, oilpan-r...@chromium.org, v8-re...@googlegroups.com
Attention needed from Dominik Inführ
Michael Lippautz added 5 comments![]( "Open in Gerrit")
File src/heap/cppgc-js/cpp-snapshot.h
Line 24, Patchset 9 (Parent):
UnorderedCppHeapExternalObjectSet&& cpp_heap_external_objects);Michael Lippautz . resolved
I forgot that we already had this...
File src/heap/cppgc-js/cpp-snapshot.cc
File src/profiler/heap-snapshot-generator-inl.h
Line 1, Patchset 9 (Latest):
// Copyright 2013 the V8 project authors. All rights reserved.Michael Lippautz . unresolved
Completely unrelated follow up request: Snapshots are not on the super critical perf path. Can we get rid of the inline header and just move all these accessors to .cc?
File src/profiler/heap-snapshot-generator.h
Line 729, Patchset 9 (Latest):
CppHeapWrapperSet& GetCppHeapWrappers() { return cpp_heap_wrappers_; }Michael Lippautz . unresolved
Personally, I really dislike the name "wrapper" here if we are allowing more and more arbitrary references back and forth.
C++->JS is already allowed on non-wrapper/wrappable scenarios. The global proxy setup also already forms a weird circle IIRC.
I don't have a good idea though so maybe fine as is...
Line 295, Patchset 9 (Latest):
mutable absl::flat_hash_map<SnapshotObjectId, HeapEntry*>Michael Lippautz . unresolved
Let's add a comment here why this is `mutable`. I understand that you want `GetEntryById` to work as `const` (and this makes sense) but it's probably hard to see from here.
Related details
Attention is currently required from:
- Dominik Inführ
Submit Requirements:
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dominik Inführ (Gerrit)
Mar 25, 2026, 5:18:54 AM (6 days ago) Mar 25
to Michael Lippautz, V8 LUCI CQ, Hannes Payer, devtools-...@chromium.org, mlippau...@chromium.org, oilpan-r...@chromium.org, v8-re...@googlegroups.com
Attention needed from Michael Lippautz
Dominik Inführ added 5 comments![]( "Open in Gerrit")
Patchset-level comments
File src/heap/cppgc-js/cpp-snapshot.cc
File src/profiler/heap-snapshot-generator-inl.h
Line 1, Patchset 9:
// Copyright 2013 the V8 project authors. All rights reserved.Michael Lippautz . resolved
Completely unrelated follow up request: Snapshots are not on the super critical perf path. Can we get rid of the inline header and just move all these accessors to .cc?
Dominik Inführ
+1. I will do this in a follow-up.
File src/profiler/heap-snapshot-generator.h
Line 729, Patchset 9:
CppHeapWrapperSet& GetCppHeapWrappers() { return cpp_heap_wrappers_; }Michael Lippautz . resolved
Personally, I really dislike the name "wrapper" here if we are allowing more and more arbitrary references back and forth.
C++->JS is already allowed on non-wrapper/wrappable scenarios. The global proxy setup also already forms a weird circle IIRC.
I don't have a good idea though so maybe fine as is...
Dominik Inführ
Agreed.
Line 295, Patchset 9:
mutable absl::flat_hash_map<SnapshotObjectId, HeapEntry*>Michael Lippautz . resolved
Let's add a comment here why this is `mutable`. I understand that you want `GetEntryById` to work as `const` (and this makes sense) but it's probably hard to see from here.
Attention is currently required from:
- Michael Lippautz
Submit Requirements:
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michael Lippautz (Gerrit)
Mar 25, 2026, 6:04:38 AM (6 days ago) Mar 25
to Dominik Inführ, V8 LUCI CQ, Hannes Payer, devtools-...@chromium.org, mlippau...@chromium.org, oilpan-r...@chromium.org, v8-re...@googlegroups.com
Attention needed from Dominik Inführ
Michael Lippautz voted Code-Review+1![]( "Open in Gerrit")
Attention is currently required from:
- Dominik Inführ
Submit Requirements:
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dominik Inführ (Gerrit)
Mar 25, 2026, 6:51:21 AM (6 days ago) Mar 25
to Michael Lippautz, V8 LUCI CQ, Hannes Payer, devtools-...@chromium.org, mlippau...@chromium.org, oilpan-r...@chromium.org, v8-re...@googlegroups.com
Dominik Inführ voted and added 1 comment![]( "Open in Gerrit")
Votes added by Dominik Inführ
| Commit-Queue | +2 |
1 comment
Patchset-level comments
Submit Requirements:
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
V8 LUCI CQ (Gerrit)
Mar 25, 2026, 7:44:37 AM (6 days ago) Mar 25
to Dominik Inführ, Michael Lippautz, Hannes Payer, devtools-...@chromium.org, mlippau...@chromium.org, oilpan-r...@chromium.org, v8-re...@googlegroups.com
V8 LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
Commit message:
[profiler] Emit standalone JS to cppgc edges
This CL starts to emit edges from JS --> C++ even when the C++ does not
have a back-ref to the JS object. This fixes unreachable objects in
heap snapshot when e.g. a DOM object was kept alive by a non-main
world JS object. In such cases there is JS --> C++ reference but the
C++ object does not have a direct back-ref to an e.g. extension's JS
wrapper object.
Bug: 492402253
Change-Id: I4c06f86133390427bd1147a86cc54c5aa3a4cd66
Reviewed-by: Michael Lippautz <mlip...@chromium.org>
Commit-Queue: Dominik Inführ <dinf...@chromium.org>
Cr-Commit-Position: refs/heads/main@{#106027}
Files:
- M src/heap/cppgc-js/cpp-snapshot.cc
- M src/heap/cppgc-js/cpp-snapshot.h
- M src/profiler/heap-profiler.cc
- M src/profiler/heap-profiler.h
- M src/profiler/heap-snapshot-common.h
- M src/profiler/heap-snapshot-generator-inl.h
- M src/profiler/heap-snapshot-generator.cc
- M src/profiler/heap-snapshot-generator.h
- M test/unittests/heap/cppgc-js/unified-heap-snapshot-unittest.cc
Change size: M
Delta: 9 files changed, 176 insertions(+), 53 deletions(-)
Branch: refs/heads/main
Submit Requirements:
Code-Review: +1 by Michael Lippautz
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Reply all
Reply to author
Forward
0 new messages