[api, heap] Report HeapStats on OOM through crash keys [v8/v8 : main]
Dominik Inführ (Gerrit)
Dominik Inführ voted and added 1 comment![]( "Open in Gerrit")
Votes added by Dominik Inführ
| Commit-Queue | +1 |
1 comment
PTAL, the Blink CL is in https://chromium-review.googlesource.com/c/chromium/src/+/7075072.
Related details
- Michael Lippautz
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michael Lippautz (Gerrit)
Michael Lippautz voted and added 4 comments![]( "Open in Gerrit")
Votes added by Michael Lippautz
| Code-Review | +1 |
4 comments
enum class CrashKeySize { Size32, Size64, Size256, Size1024 };I guess that's the max size on the Blink side?
if (!allocate_crash_key_string_callback_) return nullptr;`CHECK(HasCrashKeyStringCallbacks())`?
constexpr char kPrefix[] = "v8-oom-";nit: Move out prefix and use in `sizeof()` as well.
Related details
- Dominik Inführ
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)
Dominik Inführ added 2 comments![]( "Open in Gerrit")
if (!allocate_crash_key_string_callback_) return nullptr;`CHECK(HasCrashKeyStringCallbacks())`?
The already existing AddCrashKey() behaves like this as well. There are only 2 calls of that method which make use of that bailout, so we can easily change those locations when migration from AddCrashKey() to AddCrashKeyString(). I would see it as a convenience feature though. Otherwise we would always need to check HasCrashKeyStringCallbacks() first setting a crash key. Up to you to decide.
constexpr char kPrefix[] = "v8-oom-";nit: Move out prefix and use in `sizeof()` as well.
- Michael Lippautz
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)
Dominik Inführ added 1 comment![]( "Open in Gerrit")
enum class CrashKeySize { Size32, Size64, Size256, Size1024 };I guess that's the max size on the Blink side?
Yes exactly. This enum is a copy of base::debug::CrashKeySize.
Related details
- Michael Lippautz
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michael Lippautz (Gerrit)
Michael Lippautz voted and added 2 comments![]( "Open in Gerrit")
Votes added by Michael Lippautz
| Code-Review | +1 |
2 comments
enum class CrashKeySize { Size32, Size64, Size256, Size1024 };Dominik InführI guess that's the max size on the Blink side?
Yes exactly. This enum is a copy of base::debug::CrashKeySize.
Acknowledged
if (!allocate_crash_key_string_callback_) return nullptr;Dominik Inführ`CHECK(HasCrashKeyStringCallbacks())`?
The already existing AddCrashKey() behaves like this as well. There are only 2 calls of that method which make use of that bailout, so we can easily change those locations when migration from AddCrashKey() to AddCrashKeyString(). I would see it as a convenience feature though. Otherwise we would always need to check HasCrashKeyStringCallbacks() first setting a crash key. Up to you to decide.
Ah, well, I don't have a strong opinion here.
I think the better pattern would have been
```
if (!isolate->HasCrashKeyStringCallbacks()) return;
isolate->AddCrashKeyString(...); // Assuming that this works with a CHECK()
```
If other code works with the same pattern it's fine to leave as is.
Related details
- Dominik Inführ
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)
Dominik Inführ added 1 comment![]( "Open in Gerrit")
if (!allocate_crash_key_string_callback_) return nullptr;Dominik Inführ`CHECK(HasCrashKeyStringCallbacks())`?
Michael LippautzThe already existing AddCrashKey() behaves like this as well. There are only 2 calls of that method which make use of that bailout, so we can easily change those locations when migration from AddCrashKey() to AddCrashKeyString(). I would see it as a convenience feature though. Otherwise we would always need to check HasCrashKeyStringCallbacks() first setting a crash key. Up to you to decide.
Ah, well, I don't have a strong opinion here.
I think the better pattern would have been
```
if (!isolate->HasCrashKeyStringCallbacks()) return;isolate->AddCrashKeyString(...); // Assuming that this works with a CHECK()
```If other code works with the same pattern it's fine to leave as is.
That is reasonable imho. There are just 2 uses iiuc and they belong together. We can easily change that code. I've added the CHECK to the method.
Related details
- Michael Lippautz
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)
Dominik Inführ added 1 comment![]( "Open in Gerrit")
if (!allocate_crash_key_string_callback_) return nullptr;Dominik Inführ`CHECK(HasCrashKeyStringCallbacks())`?
Michael LippautzThe already existing AddCrashKey() behaves like this as well. There are only 2 calls of that method which make use of that bailout, so we can easily change those locations when migration from AddCrashKey() to AddCrashKeyString(). I would see it as a convenience feature though. Otherwise we would always need to check HasCrashKeyStringCallbacks() first setting a crash key. Up to you to decide.
Dominik InführAh, well, I don't have a strong opinion here.
I think the better pattern would have been
```
if (!isolate->HasCrashKeyStringCallbacks()) return;isolate->AddCrashKeyString(...); // Assuming that this works with a CHECK()
```If other code works with the same pattern it's fine to leave as is.
That is reasonable imho. There are just 2 uses iiuc and they belong together. We can easily change that code. I've added the CHECK to the method.
Done
Related details
- Michael Lippautz
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dominik Inführ (Gerrit)
Dominik Inführ voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Related details
- Michael Lippautz
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |