Add detail to names of HTML elements in heap snapshots [chromium/src : main]
Seth Brenith (Gerrit)
Seth Brenith added 1 comment![]( "Open in Gerrit")
Hello Kentaro, would you please review this change? There is a corresponding change in the DevTools repository ( https://crrev.com/c/5598901 ), which includes a test for the new functionality. If you approve this change, then I would land that DevTools change before this one, and finish up with a second DevTools change to enable the skipped tests. Thanks for your time and consideration!
Related details
- Kentaro Hara
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kentaro Hara (Gerrit)
Kentaro Hara added 1 comment![]( "Open in Gerrit")
Camillo: Would you take a look at the change if this makes sense from the DevTools perspective? Then I'll LGTM this.
Related details
- Camillo Bruni
- Seth Brenith
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michael Lippautz (Gerrit)
Michael Lippautz added 1 comment![]( "Open in Gerrit")
return ThreadState::Current()->CopyNameForHeapSnapshot(utf_8.c_str());`blink::NameProvider` implements `cppgc::NameTrait` which is supposed to work outside of heap snapshots as well.
We sometimes use this for debugging which would break now.
Related details
- Camillo Bruni
- Seth Brenith
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Seth Brenith (Gerrit)
Seth Brenith added 1 comment![]( "Open in Gerrit")
return ThreadState::Current()->CopyNameForHeapSnapshot(utf_8.c_str());`blink::NameProvider` implements `cppgc::NameTrait` which is supposed to work outside of heap snapshots as well.
We sometimes use this for debugging which would break now.
Oh, great point! I see now that HeapObjectHeader::GetName is called when formatting a message for a crash at [1], as well as a couple of other places.
A possible fix would be to change the contract of CopyNameForHeapSnapshot so that it returns nullptr if a heap snapshot is not in progress, and update this code to return Element::NameInHeapSnapshot in that case. Does that seem reasonable? (I added CopyNameForHeapSnapshot very recently, so I doubt any other embedders are depending on it yet.)
Related details
- Camillo Bruni
- Michael Lippautz
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michael Lippautz (Gerrit)
Michael Lippautz added 1 comment![]( "Open in Gerrit")
return ThreadState::Current()->CopyNameForHeapSnapshot(utf_8.c_str());Seth Brenith`blink::NameProvider` implements `cppgc::NameTrait` which is supposed to work outside of heap snapshots as well.
We sometimes use this for debugging which would break now.
Oh, great point! I see now that HeapObjectHeader::GetName is called when formatting a message for a crash at [1], as well as a couple of other places.
A possible fix would be to change the contract of CopyNameForHeapSnapshot so that it returns nullptr if a heap snapshot is not in progress, and update this code to return Element::NameInHeapSnapshot in that case. Does that seem reasonable? (I added CopyNameForHeapSnapshot very recently, so I doubt any other embedders are depending on it yet.)
Can you check the same condition already on the existing API?
`CopyNameForHeapSnapshot()` seems to have a rather strange contract otherwise in that you can call it at any time but it only will be reasonable if we are creating a snapshot.
Related details
- Camillo Bruni
- Seth Brenith
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Seth Brenith (Gerrit)
Seth Brenith added 1 comment![]( "Open in Gerrit")
return ThreadState::Current()->CopyNameForHeapSnapshot(utf_8.c_str());Seth Brenith`blink::NameProvider` implements `cppgc::NameTrait` which is supposed to work outside of heap snapshots as well.
We sometimes use this for debugging which would break now.
Michael LippautzOh, great point! I see now that HeapObjectHeader::GetName is called when formatting a message for a crash at [1], as well as a couple of other places.
A possible fix would be to change the contract of CopyNameForHeapSnapshot so that it returns nullptr if a heap snapshot is not in progress, and update this code to return Element::NameInHeapSnapshot in that case. Does that seem reasonable? (I added CopyNameForHeapSnapshot very recently, so I doubt any other embedders are depending on it yet.)
Can you check the same condition already on the existing API?
`CopyNameForHeapSnapshot()` seems to have a rather strange contract otherwise in that you can call it at any time but it only will be reasonable if we are creating a snapshot.
`v8::internal::HeapProfiler` has a function `bool IsTakingSnapshot()`, but there isn't a corresponding function on the public API. Would it make sense for me to add one? Then embedders could easily tell whether it's safe to call `CopyNameForHeapSnapshot`.
Related details
- Camillo Bruni
- Michael Lippautz
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michael Lippautz (Gerrit)
Michael Lippautz added 1 comment![]( "Open in Gerrit")
return ThreadState::Current()->CopyNameForHeapSnapshot(utf_8.c_str());Seth Brenith`blink::NameProvider` implements `cppgc::NameTrait` which is supposed to work outside of heap snapshots as well.
We sometimes use this for debugging which would break now.
Michael LippautzOh, great point! I see now that HeapObjectHeader::GetName is called when formatting a message for a crash at [1], as well as a couple of other places.
A possible fix would be to change the contract of CopyNameForHeapSnapshot so that it returns nullptr if a heap snapshot is not in progress, and update this code to return Element::NameInHeapSnapshot in that case. Does that seem reasonable? (I added CopyNameForHeapSnapshot very recently, so I doubt any other embedders are depending on it yet.)
Seth BrenithCan you check the same condition already on the existing API?
`CopyNameForHeapSnapshot()` seems to have a rather strange contract otherwise in that you can call it at any time but it only will be reasonable if we are creating a snapshot.
`v8::internal::HeapProfiler` has a function `bool IsTakingSnapshot()`, but there isn't a corresponding function on the public API. Would it make sense for me to add one? Then embedders could easily tell whether it's safe to call `CopyNameForHeapSnapshot`.
Seems like a better fit than allowing the copy call to be always called?
Related details
- Camillo Bruni
- Seth Brenith
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Seth Brenith (Gerrit)
Seth Brenith voted and added 1 comment![]( "Open in Gerrit")
Votes added by Seth Brenith
| Commit-Queue | +1 |
1 comment
return ThreadState::Current()->CopyNameForHeapSnapshot(utf_8.c_str());Seth Brenith`blink::NameProvider` implements `cppgc::NameTrait` which is supposed to work outside of heap snapshots as well.
We sometimes use this for debugging which would break now.
Michael LippautzOh, great point! I see now that HeapObjectHeader::GetName is called when formatting a message for a crash at [1], as well as a couple of other places.
A possible fix would be to change the contract of CopyNameForHeapSnapshot so that it returns nullptr if a heap snapshot is not in progress, and update this code to return Element::NameInHeapSnapshot in that case. Does that seem reasonable? (I added CopyNameForHeapSnapshot very recently, so I doubt any other embedders are depending on it yet.)
Seth BrenithCan you check the same condition already on the existing API?
`CopyNameForHeapSnapshot()` seems to have a rather strange contract otherwise in that you can call it at any time but it only will be reasonable if we are creating a snapshot.
Michael Lippautz`v8::internal::HeapProfiler` has a function `bool IsTakingSnapshot()`, but there isn't a corresponding function on the public API. Would it make sense for me to add one? Then embedders could easily tell whether it's safe to call `CopyNameForHeapSnapshot`.
Seems like a better fit than allowing the copy call to be always called?
Sounds good, thanks! I've updated the code here to check whether a snapshot is in progress.
Related details
- Camillo Bruni
- Michael Lippautz
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michael Lippautz (Gerrit)
Michael Lippautz voted and added 1 comment![]( "Open in Gerrit")
Votes added by Michael Lippautz
| Code-Review | +1 |
1 comment
Related details
- Camillo Bruni
- Seth Brenith
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Seth Brenith (Gerrit)
Seth Brenith voted and added 1 comment![]( "Open in Gerrit")
Votes added by Seth Brenith
| Commit-Queue | +2 |
1 comment
Thanks for reviewing, Michael!
Related details
- Camillo Bruni
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kentaro Hara (Gerrit)
Kentaro Hara voted and added 1 comment![]( "Open in Gerrit")
Votes added by Kentaro Hara
| Code-Review | +1 |
1 comment
- Camillo Bruni
- Seth Brenith
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
Add detail to names of HTML elements in heap snapshots
In heap snapshots, it can be difficult to see which HTMLElement
instances correspond to which elements. With this change, Blink will
provide a formatted start tag as the name which will be shown in the
heap snapshot.
- M third_party/blink/renderer/core/html/html_element.cc
- M third_party/blink/renderer/core/html/html_element.h
- M third_party/blink/renderer/platform/heap/thread_state.cc
- M third_party/blink/renderer/platform/heap/thread_state.h
Code-Review: +1 by Michael Lippautz, +1 by Kentaro Hara
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |