Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2008823002:
bindings: Supports [SaveSameObject] extended attribute. (Closed)
|
Created:
4 years, 7 months ago by Yuki Modified:
4 years, 6 months ago Reviewers:
haraken CC:
chromium-reviews, blink-reviews-css, dglazkov+blink, apavlov+blink_chromium.org, darktears, blink-reviews, blink-reviews-bindings_chromium.org, rwlbuis, Peter Beverloo Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionbindings: Supports [SaveSameObject] extended attribute.
Syntactically we've supported [SameObject] extended attribute,
but we've not had any implementation for it.
This CL supports [SaveSameObject] extended attribute that
stores the first returned value in the holder's private value,
and returns it for the second time.
The first attempt is: https://crrev.com/1980553002
BUG=462913
Committed: https://crrev.com/d3d78df139ae5f8bbb655975a751a17c433193b9
Cr-Commit-Position: refs/heads/master@{#396450}
Patch Set 1 : https://crrev.com/1980553002 #Patch Set 2 : Supports [DoNotSaveSameObject] to avoid performance regressions. #Patch Set 3 : Synced. #Patch Set 4 : Changed DoNotSaveSameObject to SaveSameObject. #Patch Set 5 : Fixed tests. #Patch Set 6 : Fixed the documentation. #
Messages
Total messages: 27 (10 generated)
Description was changed from
==========
bindings: Supports [SameObject] extended attribute.
Syntactically we've supported [SameObject] extended attribute,
but we've not had any implementation for it.
This CL stores the first returned value in the holder's
private value, and returns it for the second time.
BUG=462913
==========
to
==========
bindings: Supports [SameObject] extended attribute.
Syntactically we've supported [SameObject] extended attribute,
but we've not had any implementation for it.
This CL stores the first returned value in the holder's
private value, and returns it for the second time.
This CL also supports [DoNotSaveSameObject] to disable the
caching in V8 private properties because some of simplest
implementations can be much much faster than V8 private
properties.
Example of simplest implementation)
// Just retrieves an internal field.
T* impl = V8T::toImpl(info.Holder());
// Just returns a member variable.
ScriptWrappable* ret = impl->method();
// Just retrieves a wrapper from ScriptWrappable.
info.GetReturnValue().Set(toV8(ret));
This is much faster than looking up a private property.
In order to avoid a performance regression in such a case,
this CL supports [DoNotSaveSameObject].
BUG=462913
==========
Description was changed from
==========
bindings: Supports [SameObject] extended attribute.
Syntactically we've supported [SameObject] extended attribute,
but we've not had any implementation for it.
This CL stores the first returned value in the holder's
private value, and returns it for the second time.
This CL also supports [DoNotSaveSameObject] to disable the
caching in V8 private properties because some of simplest
implementations can be much much faster than V8 private
properties.
Example of simplest implementation)
// Just retrieves an internal field.
T* impl = V8T::toImpl(info.Holder());
// Just returns a member variable.
ScriptWrappable* ret = impl->method();
// Just retrieves a wrapper from ScriptWrappable.
info.GetReturnValue().Set(toV8(ret));
This is much faster than looking up a private property.
In order to avoid a performance regression in such a case,
this CL supports [DoNotSaveSameObject].
BUG=462913
==========
to
==========
bindings: Supports [SameObject] extended attribute. (2nd try)
Syntactically we've supported [SameObject] extended attribute,
but we've not had any implementation for it.
This CL stores the first returned value in the holder's
private value, and returns it for the second time.
This CL also supports [DoNotSaveSameObject] to disable the
caching in V8 private properties because some of simplest
implementations can be much much faster than V8 private
properties.
Example of simplest implementation)
// Just retrieves an internal field.
T* impl = V8T::toImpl(info.Holder());
// Just returns a member variable.
ScriptWrappable* ret = impl->method();
// Just retrieves a wrapper from ScriptWrappable.
info.GetReturnValue().Set(toV8(ret));
This is much faster than looking up a private property.
In order to avoid a performance regression in such a case,
this CL supports [DoNotSaveSameObject].
The first attempt is: https://crrev.com/1980553002
BUG=462913
==========
yukishiino@chromium.org changed reviewers: + haraken@chromium.org
Could you review this CL?
This makes a lot of more sense. Is there any use case where we want to specify [SameObject] but can't specify [DoNotSaveSameObject]? I'm wondering if we can unconditionally make it a duty of C++ implementation to return the same object.
On 2016/05/24 17:07:56, haraken wrote: > This makes a lot of more sense. > > Is there any use case where we want to specify [SameObject] but can't specify > [DoNotSaveSameObject]? I'm wondering if we can unconditionally make it a duty of > C++ implementation to return the same object. I'm dreaming an opposite direction. The binding layer should always check it, so no one can cheat. If no one checks the payment, who pay the tax? We cannot guarantee the same object *systematically*. Originally, Notification APIs need the support of [SameObject], and then we started implementing [SameObject]. See https://crrev.com/1974033003 .
On 2016/05/26 11:43:51, Yuki wrote: > On 2016/05/24 17:07:56, haraken wrote: > > This makes a lot of more sense. > > > > Is there any use case where we want to specify [SameObject] but can't specify > > [DoNotSaveSameObject]? I'm wondering if we can unconditionally make it a duty > of > > C++ implementation to return the same object. > > I'm dreaming an opposite direction. The binding layer should always check it, > so no one can cheat. If no one checks the payment, who pay the tax? We cannot > guarantee the same object *systematically*. > > Originally, Notification APIs need the support of [SameObject], and then we > started implementing [SameObject]. See https://crrev.com/1974033003 . Would it be an option to store the same object in the hidden value and check the equality only on Debug builds? On release builds, we just rely on C++ implementations to return the same object.
On 2016/05/26 16:00:58, haraken wrote: > On 2016/05/26 11:43:51, Yuki wrote: > > On 2016/05/24 17:07:56, haraken wrote: > > > This makes a lot of more sense. > > > > > > Is there any use case where we want to specify [SameObject] but can't > specify > > > [DoNotSaveSameObject]? I'm wondering if we can unconditionally make it a > duty > > of > > > C++ implementation to return the same object. > > > > I'm dreaming an opposite direction. The binding layer should always check it, > > so no one can cheat. If no one checks the payment, who pay the tax? We > cannot > > guarantee the same object *systematically*. > > > > Originally, Notification APIs need the support of [SameObject], and then we > > started implementing [SameObject]. See https://crrev.com/1974033003 . > > Would it be an option to store the same object in the hidden value and check the > equality only on Debug builds? On release builds, we just rely on C++ > implementations to return the same object. It's possible, but I don't like that idea because it changes the lifetime of the object. I don't want to introduce a behavioral change only into debug build. If only debug build holds a hidden value, the holder object retain the hidden object, and it never gets gc'ed unless the holder object gets gc'ed. I'm afraid that it would make it hard to debug lifetime-related issues.
On 2016/05/27 07:42:58, Yuki wrote: > On 2016/05/26 16:00:58, haraken wrote: > > On 2016/05/26 11:43:51, Yuki wrote: > > > On 2016/05/24 17:07:56, haraken wrote: > > > > This makes a lot of more sense. > > > > > > > > Is there any use case where we want to specify [SameObject] but can't > > specify > > > > [DoNotSaveSameObject]? I'm wondering if we can unconditionally make it a > > duty > > > of > > > > C++ implementation to return the same object. > > > > > > I'm dreaming an opposite direction. The binding layer should always check > it, > > > so no one can cheat. If no one checks the payment, who pay the tax? We > > cannot > > > guarantee the same object *systematically*. > > > > > > Originally, Notification APIs need the support of [SameObject], and then we > > > started implementing [SameObject]. See https://crrev.com/1974033003 . > > > > Would it be an option to store the same object in the hidden value and check > the > > equality only on Debug builds? On release builds, we just rely on C++ > > implementations to return the same object. > > It's possible, but I don't like that idea because it changes the lifetime of the > object. I don't want to introduce a behavioral change only into debug build. > > If only debug build holds a hidden value, the holder object retain the hidden > object, and it never gets gc'ed unless the holder object gets gc'ed. I'm afraid > that it would make it hard to debug lifetime-related issues. Yeah, you're right. Regarding the correctness (the same-object-ness), I think it would be better to test it with layout tests, not by runtime asserts. On the other hand, I still don't see much benefit in introducing [DoNotSaveSameObject]. - Until we make the private property fast enough, we should add [DoNotSaveSameObject] to all same objects. - Once we make the private property fast enough, we should drop all [DoNotSaveSameObject]s. Then what is [DoNotSaveSameObject] for?
On 2016/05/27 08:04:41, haraken wrote: > On 2016/05/27 07:42:58, Yuki wrote: > > On 2016/05/26 16:00:58, haraken wrote: > > > On 2016/05/26 11:43:51, Yuki wrote: > > > > On 2016/05/24 17:07:56, haraken wrote: > > > > > This makes a lot of more sense. > > > > > > > > > > Is there any use case where we want to specify [SameObject] but can't > > > specify > > > > > [DoNotSaveSameObject]? I'm wondering if we can unconditionally make it a > > > duty > > > > of > > > > > C++ implementation to return the same object. > > > > > > > > I'm dreaming an opposite direction. The binding layer should always check > > it, > > > > so no one can cheat. If no one checks the payment, who pay the tax? We > > > cannot > > > > guarantee the same object *systematically*. > > > > > > > > Originally, Notification APIs need the support of [SameObject], and then > we > > > > started implementing [SameObject]. See https://crrev.com/1974033003 . > > > > > > Would it be an option to store the same object in the hidden value and check > > the > > > equality only on Debug builds? On release builds, we just rely on C++ > > > implementations to return the same object. > > > > It's possible, but I don't like that idea because it changes the lifetime of > the > > object. I don't want to introduce a behavioral change only into debug build. > > > > If only debug build holds a hidden value, the holder object retain the hidden > > object, and it never gets gc'ed unless the holder object gets gc'ed. I'm > afraid > > that it would make it hard to debug lifetime-related issues. > > Yeah, you're right. Regarding the correctness (the same-object-ness), I think it > would be better to test it with layout tests, not by runtime asserts. > > On the other hand, I still don't see much benefit in introducing > [DoNotSaveSameObject]. > > - Until we make the private property fast enough, we should add > [DoNotSaveSameObject] to all same objects. > > - Once we make the private property fast enough, we should drop all > [DoNotSaveSameObject]s. > > Then what is [DoNotSaveSameObject] for? Remember that Notification APIs need the support of [SameObject]. We're blocking the ship of Notification APIs. I'm fine to flip the definition of the new extended attribute, from DoNotSaveSameObject to SaveSameObject for example.
On 2016/05/27 08:40:32, Yuki wrote: > On 2016/05/27 08:04:41, haraken wrote: > > On 2016/05/27 07:42:58, Yuki wrote: > > > On 2016/05/26 16:00:58, haraken wrote: > > > > On 2016/05/26 11:43:51, Yuki wrote: > > > > > On 2016/05/24 17:07:56, haraken wrote: > > > > > > This makes a lot of more sense. > > > > > > > > > > > > Is there any use case where we want to specify [SameObject] but can't > > > > specify > > > > > > [DoNotSaveSameObject]? I'm wondering if we can unconditionally make it > a > > > > duty > > > > > of > > > > > > C++ implementation to return the same object. > > > > > > > > > > I'm dreaming an opposite direction. The binding layer should always > check > > > it, > > > > > so no one can cheat. If no one checks the payment, who pay the tax? We > > > > cannot > > > > > guarantee the same object *systematically*. > > > > > > > > > > Originally, Notification APIs need the support of [SameObject], and then > > we > > > > > started implementing [SameObject]. See https://crrev.com/1974033003 . > > > > > > > > Would it be an option to store the same object in the hidden value and > check > > > the > > > > equality only on Debug builds? On release builds, we just rely on C++ > > > > implementations to return the same object. > > > > > > It's possible, but I don't like that idea because it changes the lifetime of > > the > > > object. I don't want to introduce a behavioral change only into debug > build. > > > > > > If only debug build holds a hidden value, the holder object retain the > hidden > > > object, and it never gets gc'ed unless the holder object gets gc'ed. I'm > > afraid > > > that it would make it hard to debug lifetime-related issues. > > > > Yeah, you're right. Regarding the correctness (the same-object-ness), I think > it > > would be better to test it with layout tests, not by runtime asserts. > > > > On the other hand, I still don't see much benefit in introducing > > [DoNotSaveSameObject]. > > > > - Until we make the private property fast enough, we should add > > [DoNotSaveSameObject] to all same objects. > > > > - Once we make the private property fast enough, we should drop all > > [DoNotSaveSameObject]s. > > > > Then what is [DoNotSaveSameObject] for? > > Remember that Notification APIs need the support of [SameObject]. We're > blocking the ship of Notification APIs. > > I'm fine to flip the definition of the new extended attribute, from > DoNotSaveSameObject to SaveSameObject for example. Ah, now I understand the problem! The Notification API returns a different object from C++ but it expects the binding layer to return the same object?
On 2016/05/27 08:48:47, haraken wrote: > On 2016/05/27 08:40:32, Yuki wrote: > > On 2016/05/27 08:04:41, haraken wrote: > > > On 2016/05/27 07:42:58, Yuki wrote: > > > > On 2016/05/26 16:00:58, haraken wrote: > > > > > On 2016/05/26 11:43:51, Yuki wrote: > > > > > > On 2016/05/24 17:07:56, haraken wrote: > > > > > > > This makes a lot of more sense. > > > > > > > > > > > > > > Is there any use case where we want to specify [SameObject] but > can't > > > > > specify > > > > > > > [DoNotSaveSameObject]? I'm wondering if we can unconditionally make > it > > a > > > > > duty > > > > > > of > > > > > > > C++ implementation to return the same object. > > > > > > > > > > > > I'm dreaming an opposite direction. The binding layer should always > > check > > > > it, > > > > > > so no one can cheat. If no one checks the payment, who pay the tax? > We > > > > > cannot > > > > > > guarantee the same object *systematically*. > > > > > > > > > > > > Originally, Notification APIs need the support of [SameObject], and > then > > > we > > > > > > started implementing [SameObject]. See https://crrev.com/1974033003 . > > > > > > > > > > Would it be an option to store the same object in the hidden value and > > check > > > > the > > > > > equality only on Debug builds? On release builds, we just rely on C++ > > > > > implementations to return the same object. > > > > > > > > It's possible, but I don't like that idea because it changes the lifetime > of > > > the > > > > object. I don't want to introduce a behavioral change only into debug > > build. > > > > > > > > If only debug build holds a hidden value, the holder object retain the > > hidden > > > > object, and it never gets gc'ed unless the holder object gets gc'ed. I'm > > > afraid > > > > that it would make it hard to debug lifetime-related issues. > > > > > > Yeah, you're right. Regarding the correctness (the same-object-ness), I > think > > it > > > would be better to test it with layout tests, not by runtime asserts. > > > > > > On the other hand, I still don't see much benefit in introducing > > > [DoNotSaveSameObject]. > > > > > > - Until we make the private property fast enough, we should add > > > [DoNotSaveSameObject] to all same objects. > > > > > > - Once we make the private property fast enough, we should drop all > > > [DoNotSaveSameObject]s. > > > > > > Then what is [DoNotSaveSameObject] for? > > > > Remember that Notification APIs need the support of [SameObject]. We're > > blocking the ship of Notification APIs. > > > > I'm fine to flip the definition of the new extended attribute, from > > DoNotSaveSameObject to SaveSameObject for example. > > Ah, now I understand the problem! > > The Notification API returns a different object from C++ but it expects the > binding layer to return the same object? I think that needs to be fixed in notification APIs somehow. As far as I understand, what Notification::vibrate is doing is: - Notification::vibrate returns different vectors every time - but it expects the binding layer to always return a wrapper of the vector that Notification::vibrate returned at the first time (i.e., it expects the binding layer to ignore vectors that it returns at the second or later times) This sounds a bit nasty to me. How is it guaranteed that the vector contents returned by Notification::vibrate doesn't change over time? If it's not guaranteed, it's a problem. If it's guaranteed, Notification::vibrate should try to return the same object in the first place. Maybe the problem is that currently we don't have a way to return the same vector from C++ to the binding layer?
On 2016/05/27 08:55:00, haraken wrote: > On 2016/05/27 08:48:47, haraken wrote: > > On 2016/05/27 08:40:32, Yuki wrote: > > > On 2016/05/27 08:04:41, haraken wrote: > > > > On 2016/05/27 07:42:58, Yuki wrote: > > > > > On 2016/05/26 16:00:58, haraken wrote: > > > > > > On 2016/05/26 11:43:51, Yuki wrote: > > > > > > > On 2016/05/24 17:07:56, haraken wrote: > > > > > > > > This makes a lot of more sense. > > > > > > > > > > > > > > > > Is there any use case where we want to specify [SameObject] but > > can't > > > > > > specify > > > > > > > > [DoNotSaveSameObject]? I'm wondering if we can unconditionally > make > > it > > > a > > > > > > duty > > > > > > > of > > > > > > > > C++ implementation to return the same object. > > > > > > > > > > > > > > I'm dreaming an opposite direction. The binding layer should always > > > check > > > > > it, > > > > > > > so no one can cheat. If no one checks the payment, who pay the tax? > > > We > > > > > > cannot > > > > > > > guarantee the same object *systematically*. > > > > > > > > > > > > > > Originally, Notification APIs need the support of [SameObject], and > > then > > > > we > > > > > > > started implementing [SameObject]. See https://crrev.com/1974033003 > . > > > > > > > > > > > > Would it be an option to store the same object in the hidden value and > > > check > > > > > the > > > > > > equality only on Debug builds? On release builds, we just rely on C++ > > > > > > implementations to return the same object. > > > > > > > > > > It's possible, but I don't like that idea because it changes the > lifetime > > of > > > > the > > > > > object. I don't want to introduce a behavioral change only into debug > > > build. > > > > > > > > > > If only debug build holds a hidden value, the holder object retain the > > > hidden > > > > > object, and it never gets gc'ed unless the holder object gets gc'ed. > I'm > > > > afraid > > > > > that it would make it hard to debug lifetime-related issues. > > > > > > > > Yeah, you're right. Regarding the correctness (the same-object-ness), I > > think > > > it > > > > would be better to test it with layout tests, not by runtime asserts. > > > > > > > > On the other hand, I still don't see much benefit in introducing > > > > [DoNotSaveSameObject]. > > > > > > > > - Until we make the private property fast enough, we should add > > > > [DoNotSaveSameObject] to all same objects. > > > > > > > > - Once we make the private property fast enough, we should drop all > > > > [DoNotSaveSameObject]s. > > > > > > > > Then what is [DoNotSaveSameObject] for? > > > > > > Remember that Notification APIs need the support of [SameObject]. We're > > > blocking the ship of Notification APIs. > > > > > > I'm fine to flip the definition of the new extended attribute, from > > > DoNotSaveSameObject to SaveSameObject for example. > > > > Ah, now I understand the problem! > > > > The Notification API returns a different object from C++ but it expects the > > binding layer to return the same object? > > I think that needs to be fixed in notification APIs somehow. > > As far as I understand, what Notification::vibrate is doing is: > > - Notification::vibrate returns different vectors every time > - but it expects the binding layer to always return a wrapper of the vector that > Notification::vibrate returned at the first time (i.e., it expects the binding > layer to ignore vectors that it returns at the second or later times) > > This sounds a bit nasty to me. How is it guaranteed that the vector contents > returned by Notification::vibrate doesn't change over time? If it's not > guaranteed, it's a problem. If it's guaranteed, Notification::vibrate should try > to return the same object in the first place. > > Maybe the problem is that currently we don't have a way to return the same > vector from C++ to the binding layer? Even if you return the same HeapVector, it doesn't work, because HeapVector does not inherit from ScritpWrappable, so the binding layer doesn't know the existing wrapper at all. So, we're doing the following thing every time. const HeapVector& result = impl->foo(); // Always return the same vector. v8::Object obj = toV8(result); // Always create a new wrapper. v8SetReturnValue(obj); // Always returns a new wrapper.
On 2016/05/27 09:06:41, Yuki wrote: > On 2016/05/27 08:55:00, haraken wrote: > > On 2016/05/27 08:48:47, haraken wrote: > > > On 2016/05/27 08:40:32, Yuki wrote: > > > > On 2016/05/27 08:04:41, haraken wrote: > > > > > On 2016/05/27 07:42:58, Yuki wrote: > > > > > > On 2016/05/26 16:00:58, haraken wrote: > > > > > > > On 2016/05/26 11:43:51, Yuki wrote: > > > > > > > > On 2016/05/24 17:07:56, haraken wrote: > > > > > > > > > This makes a lot of more sense. > > > > > > > > > > > > > > > > > > Is there any use case where we want to specify [SameObject] but > > > can't > > > > > > > specify > > > > > > > > > [DoNotSaveSameObject]? I'm wondering if we can unconditionally > > make > > > it > > > > a > > > > > > > duty > > > > > > > > of > > > > > > > > > C++ implementation to return the same object. > > > > > > > > > > > > > > > > I'm dreaming an opposite direction. The binding layer should > always > > > > check > > > > > > it, > > > > > > > > so no one can cheat. If no one checks the payment, who pay the > tax? > > > > > We > > > > > > > cannot > > > > > > > > guarantee the same object *systematically*. > > > > > > > > > > > > > > > > Originally, Notification APIs need the support of [SameObject], > and > > > then > > > > > we > > > > > > > > started implementing [SameObject]. See > https://crrev.com/1974033003 > > . > > > > > > > > > > > > > > Would it be an option to store the same object in the hidden value > and > > > > check > > > > > > the > > > > > > > equality only on Debug builds? On release builds, we just rely on > C++ > > > > > > > implementations to return the same object. > > > > > > > > > > > > It's possible, but I don't like that idea because it changes the > > lifetime > > > of > > > > > the > > > > > > object. I don't want to introduce a behavioral change only into debug > > > > build. > > > > > > > > > > > > If only debug build holds a hidden value, the holder object retain the > > > > hidden > > > > > > object, and it never gets gc'ed unless the holder object gets gc'ed. > > I'm > > > > > afraid > > > > > > that it would make it hard to debug lifetime-related issues. > > > > > > > > > > Yeah, you're right. Regarding the correctness (the same-object-ness), I > > > think > > > > it > > > > > would be better to test it with layout tests, not by runtime asserts. > > > > > > > > > > On the other hand, I still don't see much benefit in introducing > > > > > [DoNotSaveSameObject]. > > > > > > > > > > - Until we make the private property fast enough, we should add > > > > > [DoNotSaveSameObject] to all same objects. > > > > > > > > > > - Once we make the private property fast enough, we should drop all > > > > > [DoNotSaveSameObject]s. > > > > > > > > > > Then what is [DoNotSaveSameObject] for? > > > > > > > > Remember that Notification APIs need the support of [SameObject]. We're > > > > blocking the ship of Notification APIs. > > > > > > > > I'm fine to flip the definition of the new extended attribute, from > > > > DoNotSaveSameObject to SaveSameObject for example. > > > > > > Ah, now I understand the problem! > > > > > > The Notification API returns a different object from C++ but it expects the > > > binding layer to return the same object? > > > > I think that needs to be fixed in notification APIs somehow. > > > > As far as I understand, what Notification::vibrate is doing is: > > > > - Notification::vibrate returns different vectors every time > > - but it expects the binding layer to always return a wrapper of the vector > that > > Notification::vibrate returned at the first time (i.e., it expects the binding > > layer to ignore vectors that it returns at the second or later times) > > > > This sounds a bit nasty to me. How is it guaranteed that the vector contents > > returned by Notification::vibrate doesn't change over time? If it's not > > guaranteed, it's a problem. If it's guaranteed, Notification::vibrate should > try > > to return the same object in the first place. > > > > Maybe the problem is that currently we don't have a way to return the same > > vector from C++ to the binding layer? > > Even if you return the same HeapVector, it doesn't work, because HeapVector does > not inherit from ScritpWrappable, so the binding layer doesn't know the existing > wrapper at all. So, we're doing the following thing every time. > > const HeapVector& result = impl->foo(); // Always return the same vector. > v8::Object obj = toV8(result); // Always create a new wrapper. > v8SetReturnValue(obj); // Always returns a new wrapper. I think the binding layer should support a way to return the same wrapper for the same vector (just like it dose for ScriptWrappable), but I guess it will need some work. So this CL LGTM. Though I'd slightly prefer flipping [DoNotSaveSameObject] to [SaveSameObject] because the default behavior should be [DoNotSaveSameObject]. (Another option might be writing custom bindings for Notification::vibrate with a TODO.)
On 2016/05/27 09:22:34, haraken wrote: > On 2016/05/27 09:06:41, Yuki wrote: > > On 2016/05/27 08:55:00, haraken wrote: > > > On 2016/05/27 08:48:47, haraken wrote: > > > > On 2016/05/27 08:40:32, Yuki wrote: > > > > > On 2016/05/27 08:04:41, haraken wrote: > > > > > > On 2016/05/27 07:42:58, Yuki wrote: > > > > > > > On 2016/05/26 16:00:58, haraken wrote: > > > > > > > > On 2016/05/26 11:43:51, Yuki wrote: > > > > > > > > > On 2016/05/24 17:07:56, haraken wrote: > > > > > > > > > > This makes a lot of more sense. > > > > > > > > > > > > > > > > > > > > Is there any use case where we want to specify [SameObject] > but > > > > can't > > > > > > > > specify > > > > > > > > > > [DoNotSaveSameObject]? I'm wondering if we can unconditionally > > > make > > > > it > > > > > a > > > > > > > > duty > > > > > > > > > of > > > > > > > > > > C++ implementation to return the same object. > > > > > > > > > > > > > > > > > > I'm dreaming an opposite direction. The binding layer should > > always > > > > > check > > > > > > > it, > > > > > > > > > so no one can cheat. If no one checks the payment, who pay the > > tax? > > > > > > > We > > > > > > > > cannot > > > > > > > > > guarantee the same object *systematically*. > > > > > > > > > > > > > > > > > > Originally, Notification APIs need the support of [SameObject], > > and > > > > then > > > > > > we > > > > > > > > > started implementing [SameObject]. See > > https://crrev.com/1974033003 > > > . > > > > > > > > > > > > > > > > Would it be an option to store the same object in the hidden value > > and > > > > > check > > > > > > > the > > > > > > > > equality only on Debug builds? On release builds, we just rely on > > C++ > > > > > > > > implementations to return the same object. > > > > > > > > > > > > > > It's possible, but I don't like that idea because it changes the > > > lifetime > > > > of > > > > > > the > > > > > > > object. I don't want to introduce a behavioral change only into > debug > > > > > build. > > > > > > > > > > > > > > If only debug build holds a hidden value, the holder object retain > the > > > > > hidden > > > > > > > object, and it never gets gc'ed unless the holder object gets gc'ed. > > > > I'm > > > > > > afraid > > > > > > > that it would make it hard to debug lifetime-related issues. > > > > > > > > > > > > Yeah, you're right. Regarding the correctness (the same-object-ness), > I > > > > think > > > > > it > > > > > > would be better to test it with layout tests, not by runtime asserts. > > > > > > > > > > > > On the other hand, I still don't see much benefit in introducing > > > > > > [DoNotSaveSameObject]. > > > > > > > > > > > > - Until we make the private property fast enough, we should add > > > > > > [DoNotSaveSameObject] to all same objects. > > > > > > > > > > > > - Once we make the private property fast enough, we should drop all > > > > > > [DoNotSaveSameObject]s. > > > > > > > > > > > > Then what is [DoNotSaveSameObject] for? > > > > > > > > > > Remember that Notification APIs need the support of [SameObject]. We're > > > > > blocking the ship of Notification APIs. > > > > > > > > > > I'm fine to flip the definition of the new extended attribute, from > > > > > DoNotSaveSameObject to SaveSameObject for example. > > > > > > > > Ah, now I understand the problem! > > > > > > > > The Notification API returns a different object from C++ but it expects > the > > > > binding layer to return the same object? > > > > > > I think that needs to be fixed in notification APIs somehow. > > > > > > As far as I understand, what Notification::vibrate is doing is: > > > > > > - Notification::vibrate returns different vectors every time > > > - but it expects the binding layer to always return a wrapper of the vector > > that > > > Notification::vibrate returned at the first time (i.e., it expects the > binding > > > layer to ignore vectors that it returns at the second or later times) > > > > > > This sounds a bit nasty to me. How is it guaranteed that the vector contents > > > returned by Notification::vibrate doesn't change over time? If it's not > > > guaranteed, it's a problem. If it's guaranteed, Notification::vibrate should > > try > > > to return the same object in the first place. > > > > > > Maybe the problem is that currently we don't have a way to return the same > > > vector from C++ to the binding layer? > > > > Even if you return the same HeapVector, it doesn't work, because HeapVector > does > > not inherit from ScritpWrappable, so the binding layer doesn't know the > existing > > wrapper at all. So, we're doing the following thing every time. > > > > const HeapVector& result = impl->foo(); // Always return the same vector. > > v8::Object obj = toV8(result); // Always create a new wrapper. > > v8SetReturnValue(obj); // Always returns a new wrapper. > > I think the binding layer should support a way to return the same wrapper for > the same vector (just like it dose for ScriptWrappable), but I guess it will > need some work. > > So this CL LGTM. Though I'd slightly prefer flipping [DoNotSaveSameObject] to > [SaveSameObject] because the default behavior should be [DoNotSaveSameObject]. > (Another option might be writing custom bindings for Notification::vibrate with > a TODO.) I'll do the flip in this CL. I thought the same idea to introduce ScriptWrappableHeapVector, but thinking that DOMArrayBuffer's case, I'm not sure if it goes well or not. Probably we wouldn't need ScriptWrappableHeapVector that much, or it will be our headache to convert HeapVector from/to ScriptWrappableHeapVector. Anyway, I doubt if we really want to support ScriptWrappableHeapVector. For Notification's CL, see PS3. https://codereview.chromium.org/1974033003/#ps40001 They implemented [SameObject] on their side, but we stopped it.
On 2016/05/27 09:28:25, Yuki wrote: > On 2016/05/27 09:22:34, haraken wrote: > > On 2016/05/27 09:06:41, Yuki wrote: > > > On 2016/05/27 08:55:00, haraken wrote: > > > > On 2016/05/27 08:48:47, haraken wrote: > > > > > On 2016/05/27 08:40:32, Yuki wrote: > > > > > > On 2016/05/27 08:04:41, haraken wrote: > > > > > > > On 2016/05/27 07:42:58, Yuki wrote: > > > > > > > > On 2016/05/26 16:00:58, haraken wrote: > > > > > > > > > On 2016/05/26 11:43:51, Yuki wrote: > > > > > > > > > > On 2016/05/24 17:07:56, haraken wrote: > > > > > > > > > > > This makes a lot of more sense. > > > > > > > > > > > > > > > > > > > > > > Is there any use case where we want to specify [SameObject] > > but > > > > > can't > > > > > > > > > specify > > > > > > > > > > > [DoNotSaveSameObject]? I'm wondering if we can > unconditionally > > > > make > > > > > it > > > > > > a > > > > > > > > > duty > > > > > > > > > > of > > > > > > > > > > > C++ implementation to return the same object. > > > > > > > > > > > > > > > > > > > > I'm dreaming an opposite direction. The binding layer should > > > always > > > > > > check > > > > > > > > it, > > > > > > > > > > so no one can cheat. If no one checks the payment, who pay > the > > > tax? > > > > > > > > > We > > > > > > > > > cannot > > > > > > > > > > guarantee the same object *systematically*. > > > > > > > > > > > > > > > > > > > > Originally, Notification APIs need the support of > [SameObject], > > > and > > > > > then > > > > > > > we > > > > > > > > > > started implementing [SameObject]. See > > > https://crrev.com/1974033003 > > > > . > > > > > > > > > > > > > > > > > > Would it be an option to store the same object in the hidden > value > > > and > > > > > > check > > > > > > > > the > > > > > > > > > equality only on Debug builds? On release builds, we just rely > on > > > C++ > > > > > > > > > implementations to return the same object. > > > > > > > > > > > > > > > > It's possible, but I don't like that idea because it changes the > > > > lifetime > > > > > of > > > > > > > the > > > > > > > > object. I don't want to introduce a behavioral change only into > > debug > > > > > > build. > > > > > > > > > > > > > > > > If only debug build holds a hidden value, the holder object retain > > the > > > > > > hidden > > > > > > > > object, and it never gets gc'ed unless the holder object gets > gc'ed. > > > > > > I'm > > > > > > > afraid > > > > > > > > that it would make it hard to debug lifetime-related issues. > > > > > > > > > > > > > > Yeah, you're right. Regarding the correctness (the > same-object-ness), > > I > > > > > think > > > > > > it > > > > > > > would be better to test it with layout tests, not by runtime > asserts. > > > > > > > > > > > > > > On the other hand, I still don't see much benefit in introducing > > > > > > > [DoNotSaveSameObject]. > > > > > > > > > > > > > > - Until we make the private property fast enough, we should add > > > > > > > [DoNotSaveSameObject] to all same objects. > > > > > > > > > > > > > > - Once we make the private property fast enough, we should drop all > > > > > > > [DoNotSaveSameObject]s. > > > > > > > > > > > > > > Then what is [DoNotSaveSameObject] for? > > > > > > > > > > > > Remember that Notification APIs need the support of [SameObject]. > We're > > > > > > blocking the ship of Notification APIs. > > > > > > > > > > > > I'm fine to flip the definition of the new extended attribute, from > > > > > > DoNotSaveSameObject to SaveSameObject for example. > > > > > > > > > > Ah, now I understand the problem! > > > > > > > > > > The Notification API returns a different object from C++ but it expects > > the > > > > > binding layer to return the same object? > > > > > > > > I think that needs to be fixed in notification APIs somehow. > > > > > > > > As far as I understand, what Notification::vibrate is doing is: > > > > > > > > - Notification::vibrate returns different vectors every time > > > > - but it expects the binding layer to always return a wrapper of the > vector > > > that > > > > Notification::vibrate returned at the first time (i.e., it expects the > > binding > > > > layer to ignore vectors that it returns at the second or later times) > > > > > > > > This sounds a bit nasty to me. How is it guaranteed that the vector > contents > > > > returned by Notification::vibrate doesn't change over time? If it's not > > > > guaranteed, it's a problem. If it's guaranteed, Notification::vibrate > should > > > try > > > > to return the same object in the first place. > > > > > > > > Maybe the problem is that currently we don't have a way to return the same > > > > vector from C++ to the binding layer? > > > > > > Even if you return the same HeapVector, it doesn't work, because HeapVector > > does > > > not inherit from ScritpWrappable, so the binding layer doesn't know the > > existing > > > wrapper at all. So, we're doing the following thing every time. > > > > > > const HeapVector& result = impl->foo(); // Always return the same > vector. > > > v8::Object obj = toV8(result); // Always create a new wrapper. > > > v8SetReturnValue(obj); // Always returns a new wrapper. > > > > I think the binding layer should support a way to return the same wrapper for > > the same vector (just like it dose for ScriptWrappable), but I guess it will > > need some work. > > > > So this CL LGTM. Though I'd slightly prefer flipping [DoNotSaveSameObject] to > > [SaveSameObject] because the default behavior should be [DoNotSaveSameObject]. > > (Another option might be writing custom bindings for Notification::vibrate > with > > a TODO.) > > I'll do the flip in this CL. > > I thought the same idea to introduce ScriptWrappableHeapVector, but thinking > that DOMArrayBuffer's case, I'm not sure if it goes well or not. Probably we > wouldn't need ScriptWrappableHeapVector that much, or it will be our headache to > convert HeapVector from/to ScriptWrappableHeapVector. Anyway, I doubt if we > really want to support ScriptWrappableHeapVector. > > For Notification's CL, see PS3. > https://codereview.chromium.org/1974033003/#ps40001 > They implemented [SameObject] on their side, but we stopped it. Or the binding layer creates an array (only one array per interface), pass it to C++ and ask it to fill in the array. Let's land this CL for now and chat offline next week :)
The CQ bit was checked by yukishiino@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org Link to the patchset: https://codereview.chromium.org/2008823002/#ps100001 (title: "Fixed the documentation.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2008823002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2008823002/100001
The CQ bit was unchecked by yukishiino@chromium.org
Description was changed from
==========
bindings: Supports [SameObject] extended attribute. (2nd try)
Syntactically we've supported [SameObject] extended attribute,
but we've not had any implementation for it.
This CL stores the first returned value in the holder's
private value, and returns it for the second time.
This CL also supports [DoNotSaveSameObject] to disable the
caching in V8 private properties because some of simplest
implementations can be much much faster than V8 private
properties.
Example of simplest implementation)
// Just retrieves an internal field.
T* impl = V8T::toImpl(info.Holder());
// Just returns a member variable.
ScriptWrappable* ret = impl->method();
// Just retrieves a wrapper from ScriptWrappable.
info.GetReturnValue().Set(toV8(ret));
This is much faster than looking up a private property.
In order to avoid a performance regression in such a case,
this CL supports [DoNotSaveSameObject].
The first attempt is: https://crrev.com/1980553002
BUG=462913
==========
to
==========
bindings: Supports [SaveSameObject] extended attribute.
Syntactically we've supported [SameObject] extended attribute,
but we've not had any implementation for it.
This CL supports [SaveSameObject] extended attribute that
stores the first returned value in the holder's private value,
and returns it for the second time.
The first attempt is: https://crrev.com/1980553002
BUG=462913
==========
The CQ bit was checked by yukishiino@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2008823002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2008823002/100001
Message was sent while issue was closed.
Description was changed from ========== bindings: Supports [SaveSameObject] extended attribute. Syntactically we've supported [SameObject] extended attribute, but we've not had any implementation for it. This CL supports [SaveSameObject] extended attribute that stores the first returned value in the holder's private value, and returns it for the second time. The first attempt is: https://crrev.com/1980553002 BUG=462913 ========== to ========== bindings: Supports [SaveSameObject] extended attribute. Syntactically we've supported [SameObject] extended attribute, but we've not had any implementation for it. This CL supports [SaveSameObject] extended attribute that stores the first returned value in the holder's private value, and returns it for the second time. The first attempt is: https://crrev.com/1980553002 BUG=462913 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== bindings: Supports [SaveSameObject] extended attribute. Syntactically we've supported [SameObject] extended attribute, but we've not had any implementation for it. This CL supports [SaveSameObject] extended attribute that stores the first returned value in the holder's private value, and returns it for the second time. The first attempt is: https://crrev.com/1980553002 BUG=462913 ========== to ========== bindings: Supports [SaveSameObject] extended attribute. Syntactically we've supported [SameObject] extended attribute, but we've not had any implementation for it. This CL supports [SaveSameObject] extended attribute that stores the first returned value in the holder's private value, and returns it for the second time. The first attempt is: https://crrev.com/1980553002 BUG=462913 Committed: https://crrev.com/d3d78df139ae5f8bbb655975a751a17c433193b9 Cr-Commit-Position: refs/heads/master@{#396450} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/d3d78df139ae5f8bbb655975a751a17c433193b9 Cr-Commit-Position: refs/heads/master@{#396450} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
