[WIP] ReadableStream V8 extension

Initial attempts at a V8 extension to implement the ReadableStream class.

BUG=

[Dmitry Lomov (no reviews), 2015-02-13 16:39:56]

dslomov@chromium.org changed reviewers:
+ dslomov@chromium.org

[Dmitry Lomov (no reviews), 2015-02-13 16:39:56]

https://codereview.chromium.org/924713002/diff/1/Source/platform/streams/WebS...
File Source/platform/streams/WebStreams.cpp (right):

https://codereview.chromium.org/924713002/diff/1/Source/platform/streams/WebS...
Source/platform/streams/WebStreams.cpp:80: auto privateSymbol =
v8::Private::ForApi(isolate, name);
Hmm I do not get it. Seems like you are creating a new private symbol for every
get and set, don't you?
So
  var o = {}
  // creates a new private symbol `p` and sets o[p] to 42
  SET_PRIVATE(o, "foo", 42); 
  // always undefined! why? this creates a new private symbol `p1` and does
o[p1]
  // since it is a fresh private symbol, it is undefined.
  GET_PRIVATE(o, "foo") 
                                     

What you should do instead is to create and expose the private symbols you need
and then just use normal o[MyPrivateSymbol] and o[MyPrivateSymbol] = 42.

[domenic (use chromium.org), 2015-02-13 16:42:21]

The description of v8::Private::ForApi says it will only create one with a given
name if it does not exist yet.

I would like to be able to create private symbols and use them directly, but
v8::Private inherits from v8::Data and not v8::Value, so I cannot find a way to
smuggle it into the JS code.

[Dmitry Lomov (no reviews), 2015-02-13 19:21:44]

On 2015/02/13 16:42:21, domenic wrote:
> The description of v8::Private::ForApi says it will only create one with a
given
> name if it does not exist yet.
> 
> I would like to be able to create private symbols and use them directly, but
> v8::Private inherits from v8::Data and not v8::Value, so I cannot find a way
to
> smuggle it into the JS code.

Oh, I see. Weird, internally it is just a symbol. Andreas?

[Dmitry Lomov (no reviews), 2015-02-13 19:21:57]

dslomov@chromium.org changed reviewers:
+ rossberg@chromium.org

[Dmitry Lomov (no reviews), 2015-02-13 19:46:09]

On 2015/02/13 19:21:44, Dmitry Lomov (chromium) wrote:
> On 2015/02/13 16:42:21, domenic wrote:
> > The description of v8::Private::ForApi says it will only create one with a
> given
> > name if it does not exist yet.
> > 
> > I would like to be able to create private symbols and use them directly, but
> > v8::Private inherits from v8::Data and not v8::Value, so I cannot find a way
> to
> > smuggle it into the JS code.
> 
> Oh, I see. Weird, internally it is just a symbol. Andreas?

Ok, actually after thinking about this for a bit, I figured that exposing
v8::Private to JS would not be a good idea.
Private symbols have weird semantics. If for some embedder (even if that
embedder is not Blink) they will leak into JS, that will expose weird private
symbol semantics to JS as well.

So GET_PRIVATE and SET_PRIVATE might be a reasonable approach. However what I
would rather suggest here is to have a set of higher-level operations on streams
that would use v8::Private internally. For example let's say you want to
maintain Offset in a stream (no idea if that makes sense or not). With
GET_PRIVATE and SET_PRIVATE you would use  GET_PRIVATE(stream, "offset") and
SET_PRIVATE(stream, "offset", value).
I suggest having to expose GetOffset and SetOffset instead for this purpose (and
those will use a private symbol).

I am also kind of unhappy with the API here anyway. You should be able to keep a
persistent reference to v8::Private and use that instead of getting v8::Private
by name all the time.

WDYT?

[domenic (use chromium.org), 2015-02-13 19:59:06]

On 2015/02/13 at 19:46:09, dslomov wrote:
> On 2015/02/13 19:21:44, Dmitry Lomov (chromium) wrote:
> > On 2015/02/13 16:42:21, domenic wrote:
> > > The description of v8::Private::ForApi says it will only create one with a
> > given
> > > name if it does not exist yet.
> > > 
> > > I would like to be able to create private symbols and use them directly,
but
> > > v8::Private inherits from v8::Data and not v8::Value, so I cannot find a
way
> > to
> > > smuggle it into the JS code.
> > 
> > Oh, I see. Weird, internally it is just a symbol. Andreas?
> 
> Ok, actually after thinking about this for a bit, I figured that exposing
v8::Private to JS would not be a good idea.
> Private symbols have weird semantics. If for some embedder (even if that
embedder is not Blink) they will leak into JS, that will expose weird private
symbol semantics to JS as well.

Yeah I can totally understand this.

> So GET_PRIVATE and SET_PRIVATE might be a reasonable approach. However what I
would rather suggest here is to have a set of higher-level operations on streams
that would use v8::Private internally. For example let's say you want to
maintain Offset in a stream (no idea if that makes sense or not). With
GET_PRIVATE and SET_PRIVATE you would use  GET_PRIVATE(stream, "offset") and
SET_PRIVATE(stream, "offset", value).
> I suggest having to expose GetOffset and SetOffset instead for this purpose
(and those will use a private symbol).

I dunno, I could but it seems a bit redundant to be writing a bunch of wrapper
functions `function getX(obj) { return GET_PRIVATE(obj, 'ReadableStream#X'); }`.
Given the extensive test suite I already have from streams I think the move from
strings to identifiers wouldn't stave off any bugs that wouldn't be caught
anyway.

> I am also kind of unhappy with the API here anyway. You should be able to keep
a persistent reference to v8::Private and use that instead of getting
v8::Private by name all the time.

This is my biggest concern, and is what Arv originally raised. Every time I do
v8::Private::ForApi it will do a hashtable lookup. I would much prefer to get
the same speed V8 built-ins get with their simple `obj[privateSymbol]` access.
Instead I am doing roughly the equivalent of `obj[Symbol.for('string')]`, plus
whatever the cost is for the indirection into the C++-implemented native
function. (Maybe that is not actually a cost, I don't know.)

[Dmitry Lomov (no reviews), 2015-02-13 20:02:17]

On 2015/02/13 19:59:06, domenic wrote:
> On 2015/02/13 at 19:46:09, dslomov wrote:
> > On 2015/02/13 19:21:44, Dmitry Lomov (chromium) wrote:
> > > On 2015/02/13 16:42:21, domenic wrote:
> > > > The description of v8::Private::ForApi says it will only create one with
a
> > > given
> > > > name if it does not exist yet.
> > > > 
> > > > I would like to be able to create private symbols and use them directly,
> but
> > > > v8::Private inherits from v8::Data and not v8::Value, so I cannot find a
> way
> > > to
> > > > smuggle it into the JS code.
> > > 
> > > Oh, I see. Weird, internally it is just a symbol. Andreas?
> > 
> > Ok, actually after thinking about this for a bit, I figured that exposing
> v8::Private to JS would not be a good idea.
> > Private symbols have weird semantics. If for some embedder (even if that
> embedder is not Blink) they will leak into JS, that will expose weird private
> symbol semantics to JS as well.
> 
> Yeah I can totally understand this.
> 
> > So GET_PRIVATE and SET_PRIVATE might be a reasonable approach. However what
I
> would rather suggest here is to have a set of higher-level operations on
streams
> that would use v8::Private internally. For example let's say you want to
> maintain Offset in a stream (no idea if that makes sense or not). With
> GET_PRIVATE and SET_PRIVATE you would use  GET_PRIVATE(stream, "offset") and
> SET_PRIVATE(stream, "offset", value).
> > I suggest having to expose GetOffset and SetOffset instead for this purpose
> (and those will use a private symbol).
> 
> I dunno, I could but it seems a bit redundant to be writing a bunch of wrapper
> functions `function getX(obj) { return GET_PRIVATE(obj, 'ReadableStream#X');
}`.
> Given the extensive test suite I already have from streams I think the move
from
> strings to identifiers wouldn't stave off any bugs that wouldn't be caught
> anyway.
> 
> > I am also kind of unhappy with the API here anyway. You should be able to
keep
> a persistent reference to v8::Private and use that instead of getting
> v8::Private by name all the time.
> 
> This is my biggest concern, and is what Arv originally raised. Every time I do
> v8::Private::ForApi it will do a hashtable lookup. I would much prefer to get
> the same speed V8 built-ins get with their simple `obj[privateSymbol]` access.
> Instead I am doing roughly the equivalent of `obj[Symbol.for('string')]`, plus
> whatever the cost is for the indirection into the C++-implemented native
> function. (Maybe that is not actually a cost, I don't know.)

I think we should have the ability to cache v8::Private and then you'd need
special getters/setters instead of GET_PRIVATE/SET_PRIVATE anyway.
Andreas is the author of the API though, so I'd like to hear his opinion on this
as well.

[rossberg, 2015-02-16 13:37:12]

On 2015/02/13 20:02:17, Dmitry Lomov (chromium) wrote:
> On 2015/02/13 19:59:06, domenic wrote:
> > On 2015/02/13 at 19:46:09, dslomov wrote:
> > > On 2015/02/13 19:21:44, Dmitry Lomov (chromium) wrote:
> > > > On 2015/02/13 16:42:21, domenic wrote:
> > > > > The description of v8::Private::ForApi says it will only create one
with
> a
> > > > given
> > > > > name if it does not exist yet.
> > > > > 
> > > > > I would like to be able to create private symbols and use them
directly,
> > but
> > > > > v8::Private inherits from v8::Data and not v8::Value, so I cannot find
a
> > way
> > > > to
> > > > > smuggle it into the JS code.
> > > > 
> > > > Oh, I see. Weird, internally it is just a symbol. Andreas?
> > > 
> > > Ok, actually after thinking about this for a bit, I figured that exposing
> > v8::Private to JS would not be a good idea.
> > > Private symbols have weird semantics. If for some embedder (even if that
> > embedder is not Blink) they will leak into JS, that will expose weird
private
> > symbol semantics to JS as well.
> > 
> > Yeah I can totally understand this.
> > 
> > > So GET_PRIVATE and SET_PRIVATE might be a reasonable approach. However
what
> I
> > would rather suggest here is to have a set of higher-level operations on
> streams
> > that would use v8::Private internally. For example let's say you want to
> > maintain Offset in a stream (no idea if that makes sense or not). With
> > GET_PRIVATE and SET_PRIVATE you would use  GET_PRIVATE(stream, "offset") and
> > SET_PRIVATE(stream, "offset", value).
> > > I suggest having to expose GetOffset and SetOffset instead for this
purpose
> > (and those will use a private symbol).
> > 
> > I dunno, I could but it seems a bit redundant to be writing a bunch of
wrapper
> > functions `function getX(obj) { return GET_PRIVATE(obj, 'ReadableStream#X');
> }`.
> > Given the extensive test suite I already have from streams I think the move
> from
> > strings to identifiers wouldn't stave off any bugs that wouldn't be caught
> > anyway.
> > 
> > > I am also kind of unhappy with the API here anyway. You should be able to
> keep
> > a persistent reference to v8::Private and use that instead of getting
> > v8::Private by name all the time.
> > 
> > This is my biggest concern, and is what Arv originally raised. Every time I
do
> > v8::Private::ForApi it will do a hashtable lookup. I would much prefer to
get
> > the same speed V8 built-ins get with their simple `obj[privateSymbol]`
access.
> > Instead I am doing roughly the equivalent of `obj[Symbol.for('string')]`,
plus
> > whatever the cost is for the indirection into the C++-implemented native
> > function. (Maybe that is not actually a cost, I don't know.)
> 
> I think we should have the ability to cache v8::Private and then you'd need
> special getters/setters instead of GET_PRIVATE/SET_PRIVATE anyway.
> Andreas is the author of the API though, so I'd like to hear his opinion on
this
> as well.

The API intentionally hides the fact that privates are symbols, or even JS
values, and prevents them from leaking into JS code. The rationale was to
abstract away from, and stay forwards-compatible with whatever means we
ultimately get in ES to express private state officially.

You shouldn't be going through Private::ForApi for this use case. Rather, define
a native getter/setter pair for each private, with the respective private
attached as the 'data' value (the API's way to emulate a closure).
...Unfortunately, I just realised that that won't work, because data is
restricted to type Value for no obvious reason, and this can't be changed
easily.

Hm, I have to think about this. In the meantime, I recommend just using ES6
symbols, which seem good enough for a prototype.

[domenic (use chromium.org), 2015-04-27 12:59:17]

d@domenic.me changed reviewers:
+ jochen@chromium.org, tyoshino@chromium.org

[domenic (use chromium.org), 2015-04-30 10:10:29]

Closing in favor of https://codereview.chromium.org/1118673002 which is getting
more serious.