Create Simplified Heap Iteration for Detached Elements [v8/v8 : main]
Robert Paveza (Gerrit)
Robert Paveza added 3 comments![]( "Open in Gerrit")
Nit: Probably don't need to delete this line
TEST_F(UnifiedHeapSnapshotTest, DetachedJSObjects) {Consider adding a comment at the prolog to this test explaining precisely what you're testing. Alternatively, change the name of the test (which might be preferable). Even the comments within your test don't clarify that you're solving the problem of retained external objects.
Related details
- Giovanni Del Valle
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Giovanni Del Valle (Gerrit)
Giovanni Del Valle added 7 comments![]( "Open in Gerrit")
Change-Id: I5dd543b961a19fd3e3252eba2385b52127927be7Giovanni Del ValleThese things (from this line onward) probably should not exist as part of your commit message.
Done
virtual std::vector<v8::Local<v8::Data>> getDetachedNodes() = 0;Giovanni Del ValleYou probably don't want to use the term `nodes` here -- probably `externalObjects` or `embedderObjects` would be more appropriate. `Nodes` is a Blink-ism.
Done
#define V8_BUILD_NUMBER 52Giovanni Del ValleBeware this file. You don't want to be changing it.
Done
// Keep only Detached ElementsGiovanni Del ValleMentioned in your other CL: calling this "detached elements" is violating layering. This can exist in any embedder.
Done
Nit: Probably don't need to delete this line
Done
Consider adding a comment at the prolog to this test explaining precisely what you're testing. Alternatively, change the name of the test (which might be preferable). Even the comments within your test don't clarify that you're solving the problem of retained external objects.
- Camillo Bruni
- Robert Paveza
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Giovanni Del Valle (Gerrit)
Giovanni Del Valle added 1 comment![]( "Open in Gerrit")
Hello, could you PTAL at this CL? You were suggested to me as an owner for this CL. Thanks.
Related details
- Camillo Bruni
- Robert Paveza
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Simon Zünd (Gerrit)
Simon Zünd added 6 comments![]( "Open in Gerrit")
Mostly lgtm with some comments.
nit: Please file a tracking bug for this feature at crbug.com and add it here as a separate `Bug: <bugid>` line. You can also link to the design doc in the bug as well. This will make finding all relevant CLs across V8, Blink and DevTools easier in the future.
virtual std::vector<v8::Local<v8::Data>> getDetachedEmbedderObjects() = 0;Does this really need to be an inspector API if it's already available on the `v8::HeapProfiler`? The call-site in the corresponding blink CL (https://crrev.com/c/5634107) could call `isolate_->GetHeapProfiler()->GetDetachedJSWrapperObjects()` directly without a round trip through the inspector APIs.
return m_heapProfilerAgent->getDetachedEmbedderObjects();This can be a `nullptr` for untrusted inspector clients (e.g. inspecting extensions). We should check for that here and return an empty `std::vector` if the `m_heapProfilerAgent` is not available.
std::vector<v8::Local<v8::Data>> JSObjectsFound;nit: Please follow standard V8 naming guidelines. `js_objects_found`. Although `detached_js_wrappers` might be a better name.
// Find objects that are detachedThis and the comment on line 62 are superfluous. They don't convey any extra info so we should remove them.
Related details
- Camillo Bruni
- Giovanni Del Valle
- Robert Paveza
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Giovanni Del Valle (Gerrit)
Giovanni Del Valle added 4 comments![]( "Open in Gerrit")
virtual std::vector<v8::Local<v8::Data>> getDetachedEmbedderObjects() = 0;Does this really need to be an inspector API if it's already available on the `v8::HeapProfiler`? The call-site in the corresponding blink CL (https://crrev.com/c/5634107) could call `isolate_->GetHeapProfiler()->GetDetachedJSWrapperObjects()` directly without a round trip through the inspector APIs.
You are correct, this inspector API is unnecessary. I will remove it and call the API directly from the Heap Profiler.
return m_heapProfilerAgent->getDetachedEmbedderObjects();This can be a `nullptr` for untrusted inspector clients (e.g. inspecting extensions). We should check for that here and return an empty `std::vector` if the `m_heapProfilerAgent` is not available.
Since I've removed the unnecessary Inspector API, this code was removed.
std::vector<v8::Local<v8::Data>> JSObjectsFound;nit: Please follow standard V8 naming guidelines. `js_objects_found`. Although `detached_js_wrappers` might be a better name.
Done
// Find objects that are detachedThis and the comment on line 62 are superfluous. They don't convey any extra info so we should remove them.
- Camillo Bruni
- Robert Paveza
- Simon Zünd
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Giovanni Del Valle (Gerrit)
Giovanni Del Valle added 1 comment![]( "Open in Gerrit")
Giovanni Del Vallenit: Please file a tracking bug for this feature at crbug.com and add it here as a separate `Bug: <bugid>` line. You can also link to the design doc in the bug as well. This will make finding all relevant CLs across V8, Blink and DevTools easier in the future.
Done
Related details
- Camillo Bruni
- Robert Paveza
- Simon Zünd
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Simon Zünd (Gerrit)
Simon Zünd voted and added 2 comments![]( "Open in Gerrit")
Votes added by Simon Zünd
| Code-Review | +1 |
2 comments
lgtm from my side, but now it's mostly non inspector things so please wait for Camillo :)
Related details
- Camillo Bruni
- Giovanni Del Valle
- Robert Paveza
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Camillo Bruni (Gerrit)
Camillo Bruni added 5 comments![]( "Open in Gerrit")
- generally looking good (need some gc folks to have a look, I'm not too familiar with that system).
- Can you remove the cherry pick? We only use that for backmerges
std::vector<v8::Local<v8::Data>> js_objects_found;This can only contain `v8::Value`, might be nicer to tighten this down.
v8::Local<v8::Data> data(Can this be directly `v8::Value`?
if (detachedness != v8::EmbedderGraph::Node::Detachedness::kDetached) continue;nit: please run `git cl format`
// Test ensures that objects that are retained by a JS reference are obtained by
// the GetDetachedJSWrapperObjects() functionnit: line width
Related details
- Giovanni Del Valle
- Robert Paveza
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Camillo Bruni (Gerrit)
Camillo Bruni added 2 comments![]( "Open in Gerrit")
* \returns the list of Detached JS Wrapper Objects.nit: Feel free to drop this comment I don't think it adds anything over the method signature :).
* Obtains list of Detached JS Wrapper Objects.Please extend the comment that this runs a GC and walks the whole heap (aka isn't a particularly fast operation)
Related details
- Giovanni Del Valle
- Robert Paveza
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Giovanni Del Valle (Gerrit)
Giovanni Del Valle added 7 comments![]( "Open in Gerrit")
nit: Feel free to drop this comment I don't think it adds anything over the method signature :).
Done
Please extend the comment that this runs a GC and walks the whole heap (aka isn't a particularly fast operation)
Done
left-over whitespace change we could revert.
Done
std::vector<v8::Local<v8::Data>> js_objects_found;This can only contain `v8::Value`, might be nicer to tighten this down.
I've changed it, but I just want to clarify that you meant the vector should be a std::vector<v8::local<v8::value>>, or a std::vector<v8::value>?
Can this be directly `v8::Value`?
Done
if (detachedness != v8::EmbedderGraph::Node::Detachedness::kDetached) continue;nit: please run `git cl format`
Done
// Test ensures that objects that are retained by a JS reference are obtained by
// the GetDetachedJSWrapperObjects() functionGiovanni Del Vallenit: line width
Converted the comment to one line, then ran git cl format
Related details
- Camillo Bruni
- Robert Paveza
- Simon Zünd
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |