[test][wasm] Deprecate vector<v8::Local> [v8/v8 : main]
Nikolaos Papaspyrou (Gerrit)
Attention is currently required from: Matthias Liedtke, Michael Lippautz.
Nikolaos Papaspyrou would like Matthias Liedtke and Michael Lippautz to review this change.
[test][wasm] Deprecate vector<v8::Local>
According to V8's public API documentation, local handles (i.e.,
objects of type v8::Local<T>) "should never be allocated on the heap".
This disallows heap-allocated data structures containing instances of
v8::Local, like std::vector<v8::Local<v8::String>>.
This CL refactors a few wasm tests that use such vectors, using instead
stack-allocated std::arrays and v8::MemorySpan objects.
Bug: chromium:1454114
Change-Id: I04b467c447e2a61369678fa05606d75f971289cd
---
M test/cctest/test-js-to-wasm.cc
1 file changed, 78 insertions(+), 58 deletions(-)
To view, visit change 4994333. To unsubscribe, or for help writing mail filters, visit settings.
Nikolaos Papaspyrou (Gerrit)
Attention is currently required from: Matthias Liedtke, Michael Lippautz.
Patch set 1:Commit-Queue +1
Matthias Liedtke (Gerrit)
Attention is currently required from: Michael Lippautz, Nikolaos Papaspyrou.
Patch set 1:Code-Review +1
1 comment:
Patchset:
LGTM
To view, visit change 4994333. To unsubscribe, or for help writing mail filters, visit settings.
Nikolaos Papaspyrou (Gerrit)
Attention is currently required from: Michael Lippautz, Omer Katz.
Nikolaos Papaspyrou would like Michael Lippautz and Omer Katz to review this change.
[debug][inspector] Deprecate vector<v8::Local>
According to V8's public API documentation, local handles (i.e.,
objects of type v8::Local<T>) "should never be allocated on the heap".
This disallows heap-allocated data structures containing instances of
v8::Local, like std::vector<v8::Local<v8::String>>.
This CL continues the deprecation of such data structures from the V8
code base, using v8::LocalVector instead.
Bug: chromium:1454114
Change-Id: I90e76c85abad6be2288666bfafade1bd19352cb7
---
M src/debug/debug-interface.cc
M src/debug/debug-interface.h
M src/inspector/v8-debugger.cc
M src/inspector/v8-runtime-agent-impl.cc
M src/inspector/value-mirror.cc
M test/cctest/test-debug.cc
6 files changed, 18 insertions(+), 18 deletions(-)
To view, visit change 4995788. To unsubscribe, or for help writing mail filters, visit settings.
Nikolaos Papaspyrou (Gerrit)
Attention is currently required from: Michael Lippautz, Omer Katz.
Omer Katz (Gerrit)
Attention is currently required from: Michael Lippautz, Nikolaos Papaspyrou.
Patch set 1:Code-Review +1
1 comment:
Patchset:
lgtm
To view, visit change 4995788. To unsubscribe, or for help writing mail filters, visit settings.
Nikolaos Papaspyrou (Gerrit)
Attention is currently required from: Michael Lippautz, Omer Katz.
Nikolaos Papaspyrou would like Michael Lippautz and Omer Katz to review this change.
[inspector] Deprecate vector<v8::Local>
According to V8's public API documentation, local handles (i.e.,
objects of type v8::Local<T>) "should never be allocated on the heap".
This disallows heap-allocated data structures containing instances of
v8::Local, like std::vector<v8::Local<v8::String>>.
This CL continues the deprecation of such data structures from the V8
code base, using v8::LocalVector instead.
Bug: chromium:1454114
Change-Id: Ia91d45e4e298a670f84ba30e7582267b3e29443e
---
M src/inspector/custom-preview.cc
M src/inspector/v8-console-message.cc
M src/inspector/v8-console-message.h
M src/inspector/v8-console.cc
4 files changed, 34 insertions(+), 29 deletions(-)
To view, visit change 5000625. To unsubscribe, or for help writing mail filters, visit settings.
Nikolaos Papaspyrou (Gerrit)
Attention is currently required from: Michael Lippautz, Omer Katz.
1 comment:
File src/inspector/v8-console-message.h:
Patch Set #1, Line 54:
MemorySpanWhy can't this be a `LocalVector`? […]
As I see it, a span is a view of an array-like container. It's more general than a vector and should be preferred, if we need access to the elements but not access to the container itself.
In one of the use cases (line 120 of v8-console.cc) an `std::array` is passed; there's no need to create a `LocalVector` there.
If it's the conversion that you're concerned with, i.e. having `{v.begin(), v.end()}` instead of just `v` when passing a `LocalVector` as a `MemorySpan`, that's a fair argument. I think it would be reasonable to have implicit conversions from vectors to spans, just as `std::span` has them.
The pdfium folks also mentioned this (among other things) and I will raise it as a question in the V8 chat. IIRC, Michael wanted the interface as minimal as possible, that's why it's not there already.
To view, visit change 5000625. To unsubscribe, or for help writing mail filters, visit settings.
Omer Katz (Gerrit)
Attention is currently required from: Michael Lippautz, Nikolaos Papaspyrou.
3 comments:
File src/inspector/v8-console-message.h:
Patch Set #1, Line 54:
v8::MemorySpan`const v8::MemorySpan` since we should not be changing it.
Patch Set #1, Line 54:
MemorySpan
As I see it, a span is a view of an array-like container. […]
Ok. I suppose my real concern was to make sure the MemorySpan doesn't change while we're using it here. The LocalVector is untouched so it's not reallocating backing stores (meaning we know it's still using the same MemorySpan). Also, the MemorySpan itself will keep the backing store from moving during a GC once CSS is enabled since it's on stack, right?
In that case, I assume this is safe.
File src/inspector/v8-console-message.cc:
Patch Set #1, Line 429:
v8::MemorySpan`const v8::MemorySpan`
To view, visit change 5000625. To unsubscribe, or for help writing mail filters, visit settings.
Michael Lippautz (Gerrit)
Attention is currently required from: Nikolaos Papaspyrou.
To view, visit change 4994333. To unsubscribe, or for help writing mail filters, visit settings.
Nikolaos Papaspyrou (Gerrit)
Patch set 1:Commit-Queue +2
Nikolaos Papaspyrou (Gerrit)
Attention is currently required from: Michael Lippautz, Omer Katz.
3 comments:
File src/inspector/v8-console-message.h:
Patch Set #1, Line 54:
v8::MemorySpan`const v8::MemorySpan` since we should not be changing it.
I can add `const` if you insist, but I think there's no need. Changing a span is not possible. There's no method of `v8::MemorySpan` that cannot be applied to a `const` object. Here we're also not changing the elements, which is shown by the `const` in the span's type parameter.
Patch Set #1, Line 54:
MemorySpan
Ok. […]
The span contains just a pointer to the backing store and the length.
It cannot change the vector in any way and there are no methods changing the span itself (i.e., the pointer or the length).The backing store of a `LocalVector` (in DL builds) is allocated with `StrongRootAllocatorBase::allocate_impl` (using `base::Malloc`) and deallocated with `StrongRootAllocatorBase::deallocate_impl` (using `base::Free`), when the lifetime of the `LocalVector` ends. There's nothing in a `MemorySpan` that keeps the `LocalVector` or its backing store alive, just as it happens with `std::span` in the STL. The backing store cannot be moved during a GC because it's not GC-managed anyway.
I believe that all this is safe under CSS (or otherwise), under the constraints that:
1. spans must not survive the actual vectors, and
2. calling methods that may relocate or shrink a vector's backing store (e.g., `push_back`) invalidates all existing spans for this vector.These requirements are the same as in the STL for `std::span`.
File src/inspector/v8-console-message.cc:
Patch Set #1, Line 429:
v8::MemorySpan`const v8::MemorySpan`
Same as above.
To view, visit change 5000625. To unsubscribe, or for help writing mail filters, visit settings.
V8 LUCI CQ (Gerrit)
V8 LUCI CQ submitted this change.
[test][wasm] Deprecate vector<v8::Local>
According to V8's public API documentation, local handles (i.e.,
objects of type v8::Local<T>) "should never be allocated on the heap".
This disallows heap-allocated data structures containing instances of
v8::Local, like std::vector<v8::Local<v8::String>>.
This CL refactors a few wasm tests that use such vectors, using instead
stack-allocated std::arrays and v8::MemorySpan objects.
Bug: chromium:1454114
Change-Id: I04b467c447e2a61369678fa05606d75f971289cd
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4994333
Reviewed-by: Matthias Liedtke <mlie...@chromium.org>
Commit-Queue: Nikolaos Papaspyrou <niko...@chromium.org>
Reviewed-by: Michael Lippautz <mlip...@chromium.org>
Cr-Commit-Position: refs/heads/main@{#90737}
---
M test/cctest/test-js-to-wasm.cc
1 file changed, 78 insertions(+), 58 deletions(-)
Omer Katz (Gerrit)
Attention is currently required from: Michael Lippautz, Nikolaos Papaspyrou.
Patch set 1:Code-Review +1
4 comments:
Patchset:
lgtm
File src/inspector/v8-console-message.h:
Patch Set #1, Line 54:
MemorySpanThe span contains just a pointer to the backing store and the length. […]
Acknowledged
I thought the backing stores were GCed, but that was wrong.
Patch Set #1, Line 54:
v8::MemorySpan
I can add `const` if you insist, but I think there's no need. Changing a span is not possible. […]
Acknowledged
File src/inspector/v8-console-message.cc:
Patch Set #1, Line 429:
v8::MemorySpan
Same as above.
Acknowledged
To view, visit change 5000625. To unsubscribe, or for help writing mail filters, visit settings.
Michael Lippautz (Gerrit)
Attention is currently required from: Nikolaos Papaspyrou.
To view, visit change 5000625. To unsubscribe, or for help writing mail filters, visit settings.
Nikolaos Papaspyrou (Gerrit)
Patch set 1:Commit-Queue +2
V8 LUCI CQ (Gerrit)
V8 LUCI CQ submitted this change.
[inspector] Deprecate vector<v8::Local>
According to V8's public API documentation, local handles (i.e.,
objects of type v8::Local<T>) "should never be allocated on the heap".
This disallows heap-allocated data structures containing instances of
v8::Local, like std::vector<v8::Local<v8::String>>.
This CL continues the deprecation of such data structures from the V8
code base, using v8::LocalVector instead.
Bug: chromium:1454114
Change-Id: Ia91d45e4e298a670f84ba30e7582267b3e29443e
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/5000625
Reviewed-by: Omer Katz <omer...@chromium.org>
Reviewed-by: Michael Lippautz <mlip...@chromium.org>
Commit-Queue: Nikolaos Papaspyrou <niko...@chromium.org>
Cr-Commit-Position: refs/heads/main@{#90755}
---
M src/inspector/custom-preview.cc
M src/inspector/v8-console-message.cc
M src/inspector/v8-console-message.h
M src/inspector/v8-console.cc
4 files changed, 34 insertions(+), 29 deletions(-)
Michael Lippautz (Gerrit)
Attention is currently required from: Nikolaos Papaspyrou.
Patch set 1:Code-Review +1
Nikolaos Papaspyrou (Gerrit)
Patch set 1:Commit-Queue +2
V8 LUCI CQ (Gerrit)
V8 LUCI CQ submitted this change.
[debug][inspector] Deprecate vector<v8::Local>
According to V8's public API documentation, local handles (i.e.,
objects of type v8::Local<T>) "should never be allocated on the heap".
This disallows heap-allocated data structures containing instances of
v8::Local, like std::vector<v8::Local<v8::String>>.
This CL continues the deprecation of such data structures from the V8
code base, using v8::LocalVector instead.
Bug: chromium:1454114
Change-Id: I90e76c85abad6be2288666bfafade1bd19352cb7
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4995788
Reviewed-by: Omer Katz <omer...@chromium.org>
Reviewed-by: Michael Lippautz <mlip...@chromium.org>
Commit-Queue: Nikolaos Papaspyrou <niko...@chromium.org>
Cr-Commit-Position: refs/heads/main@{#90829}
---
M src/debug/debug-interface.cc
M src/debug/debug-interface.h
M src/inspector/v8-debugger.cc
M src/inspector/v8-runtime-agent-impl.cc
M src/inspector/value-mirror.cc
M test/cctest/test-debug.cc
6 files changed, 18 insertions(+), 18 deletions(-)