Use per-subtype v8::CppHeapPointerTags for ScriptWrappables [chromium/src : main]
Nate Chapin (Gerrit)
Nate Chapin added 6 comments![]( "Open in Gerrit")
self.inherited = NoneFeels kinda bad to add all this boilerplate in all these places.
new_interface.direct_deriveds = list(direct_derived_set)`direct_deriveds` could possible be a class member here - it doesn't need to be transferred from the IR to the final object.
next_tag = 256Should I base this from 0 and add each tag to some constexpr first tag number instead of hard coding a value here?
static constexpr v8::CppHeapPointerTag kDOMWrappersTag =Should I get rid of all usage of this in this CL? The users after this CL are the manual WrapperTypeInfo initializations.
Related details
- Andrey Kosyakov
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andrey Kosyakov (Gerrit)
Andrey Kosyakov added 11 comments![]( "Open in Gerrit")
window->GetWrapperTypeInfo());Any reason we can't use static type info here as well?
window->GetWrapperTypeInfo());ditto here, we know the static type, so let's take advantage of it?
self.inherited = NoneFeels kinda bad to add all this boilerplate in all these places.
Would it make sense to introduce a superclass to take care of this?
self.deriveds = []plural of an adjective doesn't seem to work well in English :-) subclasses, perhaps, given you already use _subclass_ below?
direct_derived_set = identifier_to_direct_derived_set.get(either move down to the only usage, or just inline there?
new_ir.max_subclass_tag = next_tag + len(new_ir.deriveds)I think the _Embedder Pointer Sandboxing_ design doc contemplated the idea of making all tags for derived interfaces to share common binary prefix with the base class, so that the tag check could be accomplished with something like `(tag && child_mask) == expected_tag`. I understand this is going to be a bit more cumbersome to produce, so perhaps can be considered for a follow-up? Basically, for every subtree we would need to round up the subtree size to the next power of two.
return next_tagmight be worth asserting we don't accidentally go beyond `max_subclass_tag` while processing children here.
next_tag = 256Should I base this from 0 and add each tag to some constexpr first tag number instead of hard coding a value here?
I think explicit numbers are totally fine.
// The return value can be null if |wrapper| is a global proxy, which points tonit: Just _The return value may be null_ and don't go into details? I assume it would as well be null if one calls this on a random v8::Object not (yet?) associated to a wrapper?
const WrapperTypeInfo* wrapper_type_info) {So, given that (1) we now require a specific type here (2) this is a tiny inlined wrapper and (3) in most cases this would be followed by a `ScriptWrappable::ToImpl<T>()` call (which doesn't add overhead), can we combine the two? I.e. just make this a template on the type being requested and return the type:
```
template<typename T> T* ToScriptWrappable()
requires std::derived_from<T, ScriptWrappable> {
return v8::Object::Unwrap<ScriptWrappable>(
isolate, wrapper,
v8::CppHeapPointerTagRange(T::GetStaticWrapperTypeInfo()->this_pointer_tag,
T::GetStaticWrapperTypeInfo()->max_subclass_pointer_tag))->ToImpl<T>();
}
```
? Basically, the idea is that if we ask callers go give us more, we could as well give them more :-)
I understand this won't work for some of the hand-crafted types, but perhaps we could at least consider an overload for "regular" types to reduce boilerplate?
static constexpr v8::CppHeapPointerTag kDOMWrappersTag =Should I get rid of all usage of this in this CL? The users after this CL are the manual WrapperTypeInfo initializations.
We should get rid of it eventually, but as long as this CL is landable without removing all usages, a follow-up is totally fine.
Related details
- Nate Chapin
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 2 comments![]( "Open in Gerrit")
Cool!
static inline T* ToWrappableUnsafe(v8::Isolate* isolate,This is the hottest method used in all the trampolines _and_ we know `V8T`. I think we should store the limits as compile-time constants there as well and use from here.
(I don't immediately see how we could do that for Wrap() as it doesn't refer to `V8T` anywhere.
Related details
- Nate Chapin
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andrey Kosyakov (Gerrit)
Andrey Kosyakov added 1 comment![]( "Open in Gerrit")
static inline T* ToWrappableUnsafe(v8::Isolate* isolate,This is the hottest method used in all the trampolines _and_ we know `V8T`. I think we should store the limits as compile-time constants there as well and use from here.
(I don't immediately see how we could do that for Wrap() as it doesn't refer to `V8T` anywhere.
I'm fairly confident compiler is already capable to apply relevant optimizations considering T::GetStaticWrapperTypeInfo() returns a const pointer to a static const structure and `ToScriptWrappable()` gets inlined. Like this: https://godbolt.org/z/bqh5sndT6
Related details
- Michael Lippautz
- Nate Chapin
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")
static inline T* ToWrappableUnsafe(v8::Isolate* isolate,Andrey KosyakovThis is the hottest method used in all the trampolines _and_ we know `V8T`. I think we should store the limits as compile-time constants there as well and use from here.
(I don't immediately see how we could do that for Wrap() as it doesn't refer to `V8T` anywhere.
I'm fairly confident compiler is already capable to apply relevant optimizations considering T::GetStaticWrapperTypeInfo() returns a const pointer to a static const structure and `ToScriptWrappable()` gets inlined. Like this: https://godbolt.org/z/bqh5sndT6
Nice!
Please make sure our code works as the small snippet :)
Related details
- Andrey Kosyakov
- Nate Chapin
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andrey Kosyakov (Gerrit)
Andrey Kosyakov added 1 comment![]( "Open in Gerrit")
static inline T* ToWrappableUnsafe(v8::Isolate* isolate,Andrey KosyakovThis is the hottest method used in all the trampolines _and_ we know `V8T`. I think we should store the limits as compile-time constants there as well and use from here.
(I don't immediately see how we could do that for Wrap() as it doesn't refer to `V8T` anywhere.
Michael LippautzI'm fairly confident compiler is already capable to apply relevant optimizations considering T::GetStaticWrapperTypeInfo() returns a const pointer to a static const structure and `ToScriptWrappable()` gets inlined. Like this: https://godbolt.org/z/bqh5sndT6
Nice!
Please make sure our code works as the small snippet :)
Sure, this needs to be verified, after all, static object being in a different translation unit may complicate things, though LTO should help.
Related details
- Michael Lippautz
- Nate Chapin
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nate Chapin (Gerrit)
Nate Chapin added 13 comments![]( "Open in Gerrit")
Any reason we can't use static type info here as well?
Done
ditto here, we know the static type, so let's take advantage of it?
Done
Andrey KosyakovFeels kinda bad to add all this boilerplate in all these places.
Would it make sense to introduce a superclass to take care of this?
Maybe, but it also looks like there's a lot of other duplicate logic. Maybe a larger cleanup pass would be appropritate.
plural of an adjective doesn't seem to work well in English :-) subclasses, perhaps, given you already use _subclass_ below?
Done
direct_derived_set = identifier_to_direct_derived_set.get(either move down to the only usage, or just inline there?
Done
new_ir.max_subclass_tag = next_tag + len(new_ir.deriveds)I think the _Embedder Pointer Sandboxing_ design doc contemplated the idea of making all tags for derived interfaces to share common binary prefix with the base class, so that the tag check could be accomplished with something like `(tag && child_mask) == expected_tag`. I understand this is going to be a bit more cumbersome to produce, so perhaps can be considered for a follow-up? Basically, for every subtree we would need to round up the subtree size to the next power of two.
Based on what mlippautz said, it sounded like that was a nice-to-have, but not necessary for v1.
might be worth asserting we don't accidentally go beyond `max_subclass_tag` while processing children here.
Added `assert next_tag == new_ir.max_subclass_tag + 1`
static inline T* ToWrappableUnsafe(v8::Isolate* isolate,This is the hottest method used in all the trampolines _and_ we know `V8T`. I think we should store the limits as compile-time constants there as well and use from here.
(I don't immediately see how we could do that for Wrap() as it doesn't refer to `V8T` anywhere.
I only saw the OP when I started working on this, not the full thread, so I went ahead and added constexpr members to V8T. I'll keep them unless it's a binary bloat problem?
static inline T* ToWrappableUnsafe(v8::Isolate* isolate,This is the hottest method used in all the trampolines _and_ we know `V8T`. I think we should store the limits as compile-time constants there as well and use from here.
(I don't immediately see how we could do that for Wrap() as it doesn't refer to `V8T` anywhere.
Yeah, `Wrap()` is not
// The return value can be null if |wrapper| is a global proxy, which points tonit: Just _The return value may be null_ and don't go into details? I assume it would as well be null if one calls this on a random v8::Object not (yet?) associated to a wrapper?
Done
const WrapperTypeInfo* wrapper_type_info) {So, given that (1) we now require a specific type here (2) this is a tiny inlined wrapper and (3) in most cases this would be followed by a `ScriptWrappable::ToImpl<T>()` call (which doesn't add overhead), can we combine the two? I.e. just make this a template on the type being requested and return the type:
```
template<typename T> T* ToScriptWrappable()
requires std::derived_from<T, ScriptWrappable> {
return v8::Object::Unwrap<ScriptWrappable>(
isolate, wrapper,
v8::CppHeapPointerTagRange(T::GetStaticWrapperTypeInfo()->this_pointer_tag,
T::GetStaticWrapperTypeInfo()->max_subclass_pointer_tag))->ToImpl<T>();
}
```
? Basically, the idea is that if we ask callers go give us more, we could as well give them more :-)I understand this won't work for some of the hand-crafted types, but perhaps we could at least consider an overload for "regular" types to reduce boilerplate?
I think this makes sense - would it be alright if I attempted it in a followup?
Andrey KosyakovShould I get rid of all usage of this in this CL? The users after this CL are the manual WrapperTypeInfo initializations.
We should get rid of it eventually, but as long as this CL is landable without removing all usages, a follow-up is totally fine.
- Andrey Kosyakov
- 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")
static inline T* ToWrappableUnsafe(v8::Isolate* isolate,Nate ChapinThis is the hottest method used in all the trampolines _and_ we know `V8T`. I think we should store the limits as compile-time constants there as well and use from here.
(I don't immediately see how we could do that for Wrap() as it doesn't refer to `V8T` anywhere.
I only saw the OP when I started working on this, not the full thread, so I went ahead and added constexpr members to V8T. I'll keep them unless it's a binary bloat problem?
Mhm, maybe we should keep it then as it's a bit easier to see that this is will not require a global load.
(Don't have a strong opinion though as we _should_ get the same performance)
Related details
- Andrey Kosyakov
- Nate Chapin
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 3 comments![]( "Open in Gerrit")
Use per-subtype v8::CppHeapPointerTags for ScriptWrappablesnit: Can you add a short description?
I think we can add the tag to swallow the binary size regression as this work was escalated.
Can you run Speedometer3 on pinpoint m1 to check for any regressions?
Related details
- Andrey Kosyakov
- Nate Chapin
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nate Chapin (Gerrit)
Nate Chapin voted and added 6 comments![]( "Open in Gerrit")
Votes added by Nate Chapin
| Commit-Queue | +1 |
6 comments
Use per-subtype v8::CppHeapPointerTags for ScriptWrappablesnit: Can you add a short description?
Done
I think we can add the tag to swallow the binary size regression as this work was escalated.
Acknowledged
Can you run Speedometer3 on pinpoint m1 to check for any regressions?
Results in progress.
new_interface.direct_deriveds = list(direct_derived_set)`direct_deriveds` could possible be a class member here - it doesn't need to be transferred from the IR to the final object.
Acknowledged
static inline T* ToWrappableUnsafe(v8::Isolate* isolate,Nate ChapinThis is the hottest method used in all the trampolines _and_ we know `V8T`. I think we should store the limits as compile-time constants there as well and use from here.
(I don't immediately see how we could do that for Wrap() as it doesn't refer to `V8T` anywhere.
Michael LippautzI only saw the OP when I started working on this, not the full thread, so I went ahead and added constexpr members to V8T. I'll keep them unless it's a binary bloat problem?
Mhm, maybe we should keep it then as it's a bit easier to see that this is will not require a global load.
(Don't have a strong opinion though as we _should_ get the same performance)
Acknowledged
const WrapperTypeInfo* wrapper_type_info) {Nate ChapinSo, given that (1) we now require a specific type here (2) this is a tiny inlined wrapper and (3) in most cases this would be followed by a `ScriptWrappable::ToImpl<T>()` call (which doesn't add overhead), can we combine the two? I.e. just make this a template on the type being requested and return the type:
```
template<typename T> T* ToScriptWrappable()
requires std::derived_from<T, ScriptWrappable> {
return v8::Object::Unwrap<ScriptWrappable>(
isolate, wrapper,
v8::CppHeapPointerTagRange(T::GetStaticWrapperTypeInfo()->this_pointer_tag,
T::GetStaticWrapperTypeInfo()->max_subclass_pointer_tag))->ToImpl<T>();
}
```
? Basically, the idea is that if we ask callers go give us more, we could as well give them more :-)I understand this won't work for some of the hand-crafted types, but perhaps we could at least consider an overload for "regular" types to reduce boilerplate?
I think this makes sense - would it be alright if I attempted it in a followup?
- Andrey Kosyakov
- Michael Lippautz
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
chromeperf@appspot.gserviceaccount.com (Gerrit)
Message from chrom...@appspot.gserviceaccount.com![]( "Open in Gerrit")
📍 Job complete.
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/15f40917810000
Related details
- Andrey Kosyakov
- Michael Lippautz
- Nate Chapin
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
chromeperf@appspot.gserviceaccount.com (Gerrit)
Message from chrom...@appspot.gserviceaccount.com![]( "Open in Gerrit")
📍 Job complete.
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/16e85764410000
Nate Chapin (Gerrit)
Nate Chapin added 1 comment![]( "Open in Gerrit")
Nate ChapinCan you run Speedometer3 on pinpoint m1 to check for any regressions?
Results in progress.
linux shows a ~0.2% regression. m1 shows a ~0.4% improvement. Looks like a wash.
Related details
- Andrey Kosyakov
- Michael Lippautz
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andrey Kosyakov (Gerrit)
Andrey Kosyakov voted and added 6 comments![]( "Open in Gerrit")
Votes added by Andrey Kosyakov
| Code-Review | +1 |
6 comments
lgtm with a bunch of nits.
Also, we figured out (at lease one) source of code bloat: usage of leaf-type specific tag prevents deduplication in case when the same underlying C++ method is called for multiple generated bindings callbacks, for example in case of mix-ins that are implemented as a common subclass (consider {Element,DocumentFragment,Document}::children() that is actually ContainerNode::children()). arm32 builds do not incur this overhead presumably because v8 sandbox is off and the entire tags business get optimized away.
public_def = SequenceNode()nit: move down to first usage? Also, consider "public_def**s**".
static_cast<v8::CppHeapPointerTag>({});nit: might be worth using {named} variables here, it may be difficult to track positional {} with a multi-line template.
self.max_subclass_tag = NoneAny chance the tag allocation logic could live on the bind_gen side? Feels like it doesn't belong to the Web IDL world.
static T* Unwrap(v8::Isolate* isolate, v8::Local<v8::Array> v8_proxy_target) {nit: consider keeping a more specific name to avoid confusion with v8's `Unwrap()` and indicate it does a very different thing under the hood?
->template ToImpl<T>();nit: I feel like this could be named differently from the overload below and don't do a ToImpl<T>() because it has no chance of knowing if tag_range corresponds to T. So how about making this one non-templated and returning a ScriptWrappable, and the one below using it and doing ToImpl<T>()?
Related details
- Michael Lippautz
- Nate Chapin
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")
lgtm with a bunch of nits.
Also, we figured out (at lease one) source of code bloat: usage of leaf-type specific tag prevents deduplication in case when the same underlying C++ method is called for multiple generated bindings callbacks, for example in case of mix-ins that are implemented as a common subclass (consider {Element,DocumentFragment,Document}::children() that is actually ContainerNode::children()). arm32 builds do not incur this overhead presumably because v8 sandbox is off and the entire tags business get optimized away.
Yeah, I think we went from:
- GetAlignedPointerFromInternalField() -> Wrap (slight improvement, IIRC)
- tag - > tage range, but using a single tag (regression in V8)
- Using tag range in this CL wthat prohibits a lot of aliasing (regression)
On arm32 it's indeed off (because the sandbox needs a lot of virtual address space).
I think the subclassing is workng as expected. It's unfortunate that these result in different implementations now but I guess it's also somewhat working as expected because we validate sub types.
Related details
- Nate Chapin
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nate Chapin (Gerrit)
Nate Chapin voted and added 5 comments![]( "Open in Gerrit")
Votes added by Nate Chapin
| Commit-Queue | +2 |
5 comments
nit: move down to first usage? Also, consider "public_def**s**".
Done
nit: might be worth using {named} variables here, it may be difficult to track positional {} with a multi-line template.
Done
Any chance the tag allocation logic could live on the bind_gen side? Feels like it doesn't belong to the Web IDL world.
Possibly. I'll look into that on the followup
static T* Unwrap(v8::Isolate* isolate, v8::Local<v8::Array> v8_proxy_target) {nit: consider keeping a more specific name to avoid confusion with v8's `Unwrap()` and indicate it does a very different thing under the hood?
`ProxyTargetToObservableArray()`
nit: I feel like this could be named differently from the overload below and don't do a ToImpl<T>() because it has no chance of knowing if tag_range corresponds to T. So how about making this one non-templated and returning a ScriptWrappable, and the one below using it and doing ToImpl<T>()?
Inlined this variant into its two callsites: the other variant of `ToScriptWrappable`, and `ToWrappableUnsafe`.
Also updated the comment of `ToWrappableUnsafe` to note that it is no longer unsafe and should be renamed or removed.
Related details
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 with unreviewed changes![]( "Open in Gerrit")
Unreviewed changes
11 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: third_party/blink/renderer/bindings/core/v8/observable_array_exotic_object_handler.h
Insertions: 2, Deletions: 2.
@@ -465,8 +465,8 @@
private:
static BackingListWrappable& ToWrappableOrDie(v8::Isolate* isolate,
v8::Local<v8::Array> target) {
- return *ObservableArrayExoticObject::Unwrap<BackingListWrappable>(isolate,
- target);
+ return *ObservableArrayExoticObject::ProxyTargetToObservableArray<
+ BackingListWrappable>(isolate, target);
}
// https://webidl.spec.whatwg.org/#observable-array-exotic-object-set-the-length
```
```
The name of the file: third_party/blink/renderer/platform/bindings/observable_array.h
Insertions: 2, Deletions: 1.
@@ -197,7 +197,8 @@
// Returns the backing list object extracted from the proxy target object
// of type JS Array.
template <typename T>
- static T* Unwrap(v8::Isolate* isolate, v8::Local<v8::Array> v8_proxy_target) {
+ static T* ProxyTargetToObservableArray(v8::Isolate* isolate,
+ v8::Local<v8::Array> v8_proxy_target) {
return ToScriptWrappable<T>(
isolate, GetBackingObjectFromProxyTarget(isolate, v8_proxy_target));
}
```
```
The name of the file: third_party/blink/renderer/bindings/scripts/bind_gen/interface.py
Insertions: 9, Deletions: 7.
@@ -6296,24 +6296,26 @@
F = FormatNode
class_like = cg_context.class_like
- public_def = SequenceNode()
func_def = CxxFuncDefNode(name=function_name,
arg_decls=[],
return_type="constexpr const WrapperTypeInfo*",
static=True)
func_def.set_base_template_vars(cg_context.template_bindings())
func_def.body.append(TextNode("return &wrapper_type_info_;"))
- public_def.append(func_def)
- public_def.append(
+ public_defs = SequenceNode()
+ public_defs.append(func_def)
+
+ public_defs.append(
TextNode("""\
static constexpr v8::CppHeapPointerTag kThisTag =
- static_cast<v8::CppHeapPointerTag>({});
+ static_cast<v8::CppHeapPointerTag>({this_tag});
static constexpr v8::CppHeapPointerTag kMaxSubclassTag =
- static_cast<v8::CppHeapPointerTag>({});
+ static_cast<v8::CppHeapPointerTag>({max_subclass_tag});
static constexpr v8::CppHeapPointerTagRange kTagRange =
v8::CppHeapPointerTagRange(kThisTag, kMaxSubclassTag);
-""".format(class_like.tag, class_like.max_subclass_tag)))
+""".format(this_tag=class_like.tag,
+ max_subclass_tag=class_like.max_subclass_tag)))
member_var_def = TextNode(
"static const WrapperTypeInfo wrapper_type_info_;")
@@ -6434,7 +6436,7 @@
if class_like.is_interface:
wrapper_type_info_def.append(F(pattern, blink_class=blink_class))
- return public_def, member_var_def, wrapper_type_info_def
+ return public_defs, member_var_def, wrapper_type_info_def
# ----------------------------------------------------------------------------
```
```
The name of the file: third_party/blink/renderer/platform/bindings/v8_interface_bridge.h
Insertions: 5, Deletions: 4.
@@ -23,12 +23,13 @@
: nullptr;
}
- // This method will cause a bad cast if called on an object of the wrong type.
- // For use only inside bindings/, and only when the type of the object is
- // absolutely certain.
+ // TODO(japhet): Now that v8::Object::Unwrap uses tag ranges to enforce
+ // type safety, this returns nullptr if types are mismatched. It is therefore
+ // no longer "unsafe", and should be renamed or merged with ToWrappable().
static inline T* ToWrappableUnsafe(v8::Isolate* isolate,
v8::Local<v8::Object> value) {
- return ToScriptWrappable<T>(isolate, value, V8T::kTagRange);
+ return v8::Object::Unwrap<ScriptWrappable>(isolate, value, V8T::kTagRange)
+ ->template ToImpl<T>();
}
static inline bool HasInstance(v8::Isolate* isolate,
```
```
The name of the file: third_party/blink/renderer/platform/bindings/script_wrappable.h
Insertions: 5, Deletions: 13.
@@ -193,21 +193,13 @@
template <typename T>
requires std::derived_from<T, ScriptWrappable>
-inline T* ToScriptWrappable(v8::Isolate* isolate,
- v8::Local<v8::Object> wrapper,
- v8::CppHeapPointerTagRange tag_range) {
- return v8::Object::Unwrap<ScriptWrappable>(isolate, wrapper, tag_range)
- ->template ToImpl<T>();
-}
-
-template <typename T>
- requires std::derived_from<T, ScriptWrappable>
T* ToScriptWrappable(v8::Isolate* isolate, v8::Local<v8::Object> wrapper) {
const WrapperTypeInfo* wrapper_type_info = T::GetStaticWrapperTypeInfo();
- return ToScriptWrappable<T>(
- isolate, wrapper,
- v8::CppHeapPointerTagRange(wrapper_type_info->this_tag,
- wrapper_type_info->max_subclass_tag));
+ return v8::Object::Unwrap<ScriptWrappable>(
+ isolate, wrapper,
+ v8::CppHeapPointerTagRange(wrapper_type_info->this_tag,
+ wrapper_type_info->max_subclass_tag))
+ ->template ToImpl<T>();
}
// Defines |GetWrapperTypeInfo| virtual method which returns the WrapperTypeInfo
```
Change information
Use per-subtype v8::CppHeapPointerTags for ScriptWrappables
v8 has recently introduced a scheme of specifying a tag when alling
v8::Object::Wrap and v8::Object::Unwrap to assert that pointers are
not having their types confused between wrapping and unwrapping.
Unwrapping takes a tag range rather than a single tag, in order to
support wrapping as a derived class but unwrapping as one of its
base classes.
This CL makes use of that scheme in the generated bindings for
ScriptWrappables. Types with inheritance are simply assigned a tag.
Inheritance trees are processed together, starting from the base
of the inheritance tree, and assigning tags in depth-first order.
Each class that acts as a base class therefore has a contiguous range
of all of its derived classes.
- M third_party/blink/renderer/bindings/core/v8/local_window_proxy.cc
- M third_party/blink/renderer/bindings/core/v8/native_value_traits_buffer_sources.cc
- M third_party/blink/renderer/bindings/core/v8/native_value_traits_impl.h
- M third_party/blink/renderer/bindings/core/v8/observable_array_exotic_object_handler.h
- M third_party/blink/renderer/bindings/core/v8/remote_window_proxy.cc
- M third_party/blink/renderer/bindings/core/v8/script_function.cc
- M third_party/blink/renderer/bindings/core/v8/serialization/v8_script_value_serializer.cc
- M third_party/blink/renderer/bindings/core/v8/worker_or_worklet_script_controller.cc
- M third_party/blink/renderer/bindings/modules/v8/v8_context_snapshot_impl.cc
- M third_party/blink/renderer/bindings/scripts/bind_gen/interface.py
- M third_party/blink/renderer/bindings/scripts/bind_gen/observable_array.py
- M third_party/blink/renderer/bindings/scripts/web_idl/async_iterator.py
- M third_party/blink/renderer/bindings/scripts/web_idl/callback_interface.py
- M third_party/blink/renderer/bindings/scripts/web_idl/idl_compiler.py
- M third_party/blink/renderer/bindings/scripts/web_idl/interface.py
- M third_party/blink/renderer/bindings/scripts/web_idl/namespace.py
- M third_party/blink/renderer/bindings/scripts/web_idl/sync_iterator.py
- M third_party/blink/renderer/core/typed_arrays/dom_array_buffer.cc
- M third_party/blink/renderer/core/typed_arrays/dom_array_buffer_view.cc
- M third_party/blink/renderer/core/typed_arrays/dom_data_view.cc
- M third_party/blink/renderer/core/typed_arrays/dom_shared_array_buffer.cc
- M third_party/blink/renderer/core/typed_arrays/dom_typed_array.cc
- M third_party/blink/renderer/core/url_pattern/url_pattern_test.cc
- M third_party/blink/renderer/platform/bindings/frozen_array_base.cc
- M third_party/blink/renderer/platform/bindings/observable_array.cc
- M third_party/blink/renderer/platform/bindings/observable_array.h
- M third_party/blink/renderer/platform/bindings/script_wrappable.h
- M third_party/blink/renderer/platform/bindings/v8_dom_wrapper.h
- M third_party/blink/renderer/platform/bindings/v8_interface_bridge.h
- M third_party/blink/renderer/platform/bindings/wrapper_type_info.cc
- M third_party/blink/renderer/platform/bindings/wrapper_type_info.h
- M third_party/blink/renderer/platform/heap/thread_state.cc
Code-Review: +1 by Andrey Kosyakov
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
luci-bisection@appspot.gserviceaccount.com (Gerrit)
Message from luci-bi...@appspot.gserviceaccount.com![]( "Open in Gerrit")
LUCI Bisection has identified this change as the cause of a test failure. See the analysis: https://ci.chromium.org/ui/p/chromium/bisection/test-analysis/b/5765981147234304
Sample build with failed test: https://ci.chromium.org/b/8745818937287910289
Affected test(s):
[ninja://chrome/test:browser_tests/ActionAndBrowserActionAPITest.TestSetBadgeTextGlobalAndTab/All.0](https://ci.chromium.org/ui/test/chromium/ninja:%2F%2Fchrome%2Ftest:browser_tests%2FActionAndBrowserActionAPITest.TestSetBadgeTextGlobalAndTab%2FAll.0?q=VHash%3Addb4f966843f3bd4)
[ninja://chrome/test:browser_tests/BrowserCommandControllerBrowserTestRefreshOnly.ExecuteShowSyncSettings](https://ci.chromium.org/ui/test/chromium/ninja:%2F%2Fchrome%2Ftest:browser_tests%2FBrowserCommandControllerBrowserTestRefreshOnly.ExecuteShowSyncSettings?q=VHash%3Addb4f966843f3bd4)
[ninja://chrome/test:browser_tests/BrowserCommandControllerBrowserTestRefreshOnly.ShowTranslateStatusChromePage](https://ci.chromium.org/ui/test/chromium/ninja:%2F%2Fchrome%2Ftest:browser_tests%2FBrowserCommandControllerBrowserTestRefreshOnly.ShowTranslateStatusChromePage?q=VHash%3Addb4f966843f3bd4)
[ninja://chrome/test:browser_tests/BrowserNavigatorTest.NavigateFromDefaultToOptionsInSameTab](https://ci.chromium.org/ui/test/chromium/ninja:%2F%2Fchrome%2Ftest:browser_tests%2FBrowserNavigatorTest.NavigateFromDefaultToOptionsInSameTab?q=VHash%3Addb4f966843f3bd4)
[ninja://chrome/test:browser_tests/ChromeMimeHandlerViewTest.DataUrl](https://ci.chromium.org/ui/test/chromium/ninja:%2F%2Fchrome%2Ftest:browser_tests%2FChromeMimeHandlerViewTest.DataUrl?q=VHash%3Addb4f966843f3bd4)
and 94 more ...
A revert for this change was not created because the builder of the failed test(s) is not being watched by gardeners.
If this is a false positive, please report it at http://b.corp.google.com/createIssue?component=1199205&description=Analysis%3A+https%3A%2F%2Fci.chromium.org%2Fui%2Fp%2Fchromium%2Fbisection%2Ftest-analysis%2Fb%2F5765981147234304&format=PLAIN&priority=P3&title=Wrongly+blamed+https%3A%2F%2Fchromium-review.googlesource.com%2Fc%2Fchromium%2Fsrc%2F%2B%2F5581408&type=BUG
Related details
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |