[debug] Add DebugScriptScope and scope position serialization [v8/v8 : main]
Simon Zünd (Gerrit)
Simon Zünd added 5 comments![]( "Open in Gerrit")
Hey Jakob,
This CL is the beginning of go/chrome-devtools:cache-scopes-design. I discussed the idea with Leszek last week. The tl;dr is that we want to avoid repeated re-parses of scripts for every conditional breakpoint or debug evaluate in the debugger. So we plan to stash all scope info that the debugger needs on the side.
This CL only serializes the start/end position per scope, but I included the full layout thats planned as a code comment.
I just wanted to get some early, general feedback, if I'm holding the various V8 pieces correctly. Kindly see the questions inline in the code.
DirectHandle<DebugScriptScopeInfo> info_;Is this ok to have as long as `DebugScriptScope` is STACK_ALLOCATED?
ZoneVector<Scope*> all_scopes(zone);Not sure if using ZoneVector here and below is necessary or desired. A normal std::vector would probably be fine? Or whats the V8 thing to use here?
base::OwnedVector<uint8_t> buffer =
base::OwnedVector<uint8_t>::NewForOverwrite(total_size);I assumed we have to write into an OwnedVector first, before copying the whole thing into a ByteArray. The reason is that we (probably?) will allocate strings for variable names that get put into the `string_table` and that could move the ByteArray?
base::WriteUnalignedValue<int32_t>(base,
static_cast<int32_t>(all_scopes.size()));Is `base::WriteUnalignedValue` / `base::ReadUnalignedValue` the way to go (the layout is not 4 byte aligned)?
Related details
- Jakob Kummerow
- Leszek Swirski
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jakob Kummerow (Gerrit)
Jakob Kummerow added 11 comments![]( "Open in Gerrit")
static_assert(sizeof(DebugScriptScope) == sizeof(void*) + 2 * sizeof(int32_t));Why is this necessary to enforce? Just to make sure it doesn't accidentally get excessively large? Please add a brief comment; or drop the `static_assert` if you don't actually care whether this needs 16 or 64 bytes.
DirectHandle<DebugScriptScopeInfo> info_;Is this ok to have as long as `DebugScriptScope` is STACK_ALLOCATED?
Yes.
The other prerequisite is that the `DebugScriptScope` doesn't outlive the `HandleScope` in which these handles were created, i.e. the following would be a bug:
```
std::optional<DebugScriptScope> scope;
{
HandleScope handles;
DirectHandle<DSSI> scope_info = ...;
scope = DebugScriptScope::FromIndex(scope_info, scope_index);
}
scope->info_->whatever(); // USE AFTER FREE
```
That said, for the use cases visible in this CL, you don't need a handle here at all: `Tagged` would be enough. You only need a handle if you require the `DebugScriptScope` to stay valid across a heap allocation (which could cause GC).
// Factory: Returns std::nullopt if scope_index is out of range.As of this CL, the need for that is not obvious: the only non-test caller performs `CHECK(scope.has_value())` immediately anyway. If other callers are coming where the out-of-range case is actually expected to happen in production, that's fine; otherwise you might as well return a plain `DebugScriptScope` and either `CHECK` or `DCHECK` for invalid (out of range) inputs.
static_assert(ScopeRecordLayout::kFlagsOffset + sizeof(uint16_t) ==I'd say these `static_assert`s are redundant with the offset definitions above, in particular because there's no guarantee that they match the code below any better than the offsets do. For example, you could accidentally use `WriteUnalignedValue<uint32_t>(...kFlagsOffset)` below, and this `static_assert` wouldn't catch that.
I'd suggest to either just drop these entirely (based on the argument that the layout-describing comment above makes it easy enough to verify that the offsets are fine), or merge them into the offsets definitions, i.e. write `kVarCountOffset = kFlagsOffset + sizeof(uint16_t)` there. A sufficiently smart IDE will still show a tooltip saying that the constexpr evaluated to `14`.
DCHECK_GE(bytes->length().value(), static_cast<int>(sizeof(int32_t)));shorter: `kInt32Size`
DCHECK_GE(scope_index, 0);
DCHECK_LT(scope_index, GetScopeCount(info));If you don't expect invalid scope_index values in regular operation (i.e. no lazy generation or silent skipping of nonexistent entries), these two DCHECKs are enough to catch bugs, and you don't need lines 94-96 (and the std::optional return value).
ZoneVector<Scope*> all_scopes(zone);Not sure if using ZoneVector here and below is necessary or desired. A normal std::vector would probably be fine? Or whats the V8 thing to use here?
Depends on your needs. A `std::vector` seems fine here.
The main benefit of a `ZoneVector` is that it's faster: it's not affected by libc++ hardening, so random accesses don't do bounds checks (in Release mode); it might also be slightly faster to allocate/grow.
The drawback of a `ZoneVector` is that it can't free any memory before the entire Zone dies, which makes it particularly wasteful when growing dynamically, because all the old backing stores it outgrew will sit around as long as the Zone lives.
total_size += ScopeRecordLayout::kFixedBaseSize;This is going to get quite a bit more complicated with the flag-conditional optional fields, right?
base::OwnedVector<uint8_t> buffer =
base::OwnedVector<uint8_t>::NewForOverwrite(total_size);I assumed we have to write into an OwnedVector first, before copying the whole thing into a ByteArray. The reason is that we (probably?) will allocate strings for variable names that get put into the `string_table` and that could move the ByteArray?
You don't have to: the job of a `Handle` is to refer to a `ByteArray` (or other `HeapObject`) that might move due to GC.
So the alternative would be:
- allocate a `ByteArray` right away
- replace `Address base` (an absolute pointer) with `uint32_t offset` (relative to the ByteArray's start)
- replace `Address offset_ptr = base + ...` value with `... = byte_array->begin() + offset + ...`, and `Address record = ...` similarly.
So while there'd be a bit of extra cost in each iteration, that's probably not slower than needing the extra `MemCopy`, and would save (peak) memory.
#ifdef DEBUG
VerifyDebugScriptScopeInfo(this, isolate);Drop the `#ifdef`, inline the called method here. This entire file is behind `#if VERIFY_HEAP`, which is the right condition. Don't worry about manually calling this; the GC will call it for you (when running with `--verify-heap`).
EXPECT_FALSE(DebugScriptScope::FromIndex(info, -1).has_value());
EXPECT_FALSE(DebugScriptScope::FromIndex(info, 1).has_value());
EXPECT_FALSE(DebugScriptScope::FromIndex(info, 100).has_value());Related to my other comments: if this test is the _only_ case where we expect to try to look up nonexistent values, then it's not worth having.
Related details
- Leszek Swirski
- Simon Zünd
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Simon Zünd (Gerrit)
Simon Zünd added 12 comments![]( "Open in Gerrit")
Thanks for the detailed review and explanations, really appreciate it!
static_assert(sizeof(DebugScriptScope) == sizeof(void*) + 2 * sizeof(int32_t));Why is this necessary to enforce? Just to make sure it doesn't accidentally get excessively large? Please add a brief comment; or drop the `static_assert` if you don't actually care whether this needs 16 or 64 bytes.
Dropped it. Missed that one on my pre-review. AGY went a bit overboard wanting to ensure that the cursor fits into 2 registers.
Jakob KummerowIs this ok to have as long as `DebugScriptScope` is STACK_ALLOCATED?
Yes.
The other prerequisite is that the `DebugScriptScope` doesn't outlive the `HandleScope` in which these handles were created, i.e. the following would be a bug:
```
std::optional<DebugScriptScope> scope;
{
HandleScope handles;
DirectHandle<DSSI> scope_info = ...;
scope = DebugScriptScope::FromIndex(scope_info, scope_index);
}
scope->info_->whatever(); // USE AFTER FREE
```That said, for the use cases visible in this CL, you don't need a handle here at all: `Tagged` would be enough. You only need a handle if you require the `DebugScriptScope` to stay valid across a heap allocation (which could cause GC).
`DebugScriptScope` will be used in `debug-scopes.cc` and `debug-evaluate.cc` where it'll be alive across allocations of `DebugEvaluateContext` / ScopeInfo / context extension objects. Will keep the DirectHandle.
// Factory: Returns std::nullopt if scope_index is out of range.As of this CL, the need for that is not obvious: the only non-test caller performs `CHECK(scope.has_value())` immediately anyway. If other callers are coming where the out-of-range case is actually expected to happen in production, that's fine; otherwise you might as well return a plain `DebugScriptScope` and either `CHECK` or `DCHECK` for invalid (out of range) inputs.
I was mostly worried about the empty script (which is valid) to not have any scope, but seems that the empty script also has a single declaration scope.
Since we can't pause in scripts that fail to parse, we are guaranteed to always have a root scope, and any other index should come from traversing the scope tree so removing the std::optional is fine.
static_assert(ScopeRecordLayout::kFlagsOffset + sizeof(uint16_t) ==I'd say these `static_assert`s are redundant with the offset definitions above, in particular because there's no guarantee that they match the code below any better than the offsets do. For example, you could accidentally use `WriteUnalignedValue<uint32_t>(...kFlagsOffset)` below, and this `static_assert` wouldn't catch that.
I'd suggest to either just drop these entirely (based on the argument that the layout-describing comment above makes it easy enough to verify that the offsets are fine), or merge them into the offsets definitions, i.e. write `kVarCountOffset = kFlagsOffset + sizeof(uint16_t)` there. A sufficiently smart IDE will still show a tooltip saying that the constexpr evaluated to `14`.
Removed
DCHECK_GE(bytes->length().value(), static_cast<int>(sizeof(int32_t)));Simon Zündshorter: `kInt32Size`
Done
DCHECK_GE(scope_index, 0);
DCHECK_LT(scope_index, GetScopeCount(info));If you don't expect invalid scope_index values in regular operation (i.e. no lazy generation or silent skipping of nonexistent entries), these two DCHECKs are enough to catch bugs, and you don't need lines 94-96 (and the std::optional return value).
Removed the `std::optional`, see the comment in header.
Jakob KummerowNot sure if using ZoneVector here and below is necessary or desired. A normal std::vector would probably be fine? Or whats the V8 thing to use here?
Depends on your needs. A `std::vector` seems fine here.
The main benefit of a `ZoneVector` is that it's faster: it's not affected by libc++ hardening, so random accesses don't do bounds checks (in Release mode); it might also be slightly faster to allocate/grow.
The drawback of a `ZoneVector` is that it can't free any memory before the entire Zone dies, which makes it particularly wasteful when growing dynamically, because all the old backing stores it outgrew will sit around as long as the Zone lives.
Going with `std::vector` for now.
This is going to get quite a bit more complicated with the flag-conditional optional fields, right?
Indeed. I was planning to mirror the flag conditions here to avoid re-allocating the resulting array as it grows.
base::OwnedVector<uint8_t> buffer =
base::OwnedVector<uint8_t>::NewForOverwrite(total_size);Jakob KummerowI assumed we have to write into an OwnedVector first, before copying the whole thing into a ByteArray. The reason is that we (probably?) will allocate strings for variable names that get put into the `string_table` and that could move the ByteArray?
You don't have to: the job of a `Handle` is to refer to a `ByteArray` (or other `HeapObject`) that might move due to GC.
So the alternative would be:
- allocate a `ByteArray` right away
- replace `Address base` (an absolute pointer) with `uint32_t offset` (relative to the ByteArray's start)
- replace `Address offset_ptr = base + ...` value with `... = byte_array->begin() + offset + ...`, and `Address record = ...` similarly.
So while there'd be a bit of extra cost in each iteration, that's probably not slower than needing the extra `MemCopy`, and would save (peak) memory.
We'll probably populate the `string_table` as we serialize the scopes since we serialize a scopes' variable directly after the basic scope info. We'd have to be careful to dereference the Handle again at the right times or build the `string_table` as a separate pass to avoid allocations. I'll give it a go.
#ifdef DEBUG
VerifyDebugScriptScopeInfo(this, isolate);Drop the `#ifdef`, inline the called method here. This entire file is behind `#if VERIFY_HEAP`, which is the right condition. Don't worry about manually calling this; the GC will call it for you (when running with `--verify-heap`).
I was thinking to keep anything that needs to know the exact layout of `numeric_data` in `debug-scope-info.cc`, otherwise we'd also have to include it there. I could `#ifdef VERIFY_HEAP` the `VerifyDebugScriptScopeInfo` helper to keep it that way or inline the helper here. No strong preference from me for either solution.
EXPECT_FALSE(DebugScriptScope::FromIndex(info, -1).has_value());
EXPECT_FALSE(DebugScriptScope::FromIndex(info, 1).has_value());
EXPECT_FALSE(DebugScriptScope::FromIndex(info, 100).has_value());Related to my other comments: if this test is the _only_ case where we expect to try to look up nonexistent values, then it's not worth having.
- Jakob Kummerow
- Leszek Swirski
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jakob Kummerow (Gerrit)
Jakob Kummerow added 2 comments![]( "Open in Gerrit")
base::WriteUnalignedValue<int32_t>(base,
static_cast<int32_t>(all_scopes.size()));Jakob KummerowIs `base::WriteUnalignedValue` / `base::ReadUnalignedValue` the way to go (the layout is not 4 byte aligned)?
Yes.
#ifdef DEBUG
VerifyDebugScriptScopeInfo(this, isolate);Simon ZündDrop the `#ifdef`, inline the called method here. This entire file is behind `#if VERIFY_HEAP`, which is the right condition. Don't worry about manually calling this; the GC will call it for you (when running with `--verify-heap`).
I was thinking to keep anything that needs to know the exact layout of `numeric_data` in `debug-scope-info.cc`, otherwise we'd also have to include it there. I could `#ifdef VERIFY_HEAP` the `VerifyDebugScriptScopeInfo` helper to keep it that way or inline the helper here. No strong preference from me for either solution.
The `#ifdef` should definitely be `VERIFY_HEAP`.
Where to put the code, I don't feel very strongly about that either. Historically, V8 has followed the convention that all object verifiers are in this file (as you can see). Keeping the layout definition local to a class-specific .cc file is a fairly convincing reason to diverge from that precedent though.
How about moving the entire `DebugScriptScopeInfoVerify` there (with a brief comment to document the reason)? Or would that lead to other difficulty?
Related details
- Leszek Swirski
- Simon Zünd
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |