bindings: Use Maybe APIs in toXXX() functions

bindings: Use Maybe APIs in toXXX() functions

BUG=462402
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=192568

[bashi, 2015-03-11 10:30:37]

bashi@chromium.org changed reviewers:
+ haraken@chromium.org, jl@opera.com, yukishiino@chromium.or

[bashi, 2015-03-11 10:30:37]

This is an WIP and depends on https://codereview.chromium.org/995203002/.

I don't intend to land this CL as-is, but would love to hear your opinions about
this change. Specifically, I'd like to ask whether we should have
macros/functions which take ExceptionState and TryCatch. WDYT?

https://codereview.chromium.org/993333002/diff/1/Source/bindings/core/v8/V8Bi...
File Source/bindings/core/v8/V8Binding.cpp (right):

https://codereview.chromium.org/993333002/diff/1/Source/bindings/core/v8/V8Bi...
Source/bindings/core/v8/V8Binding.cpp:184: int32_t result;
As seen below, there are some code which look almost the same (but slightly
different). I'm not sure we should factor out them as helper functions/macros.

https://codereview.chromium.org/993333002/diff/1/Source/bindings/core/v8/V8Bi...
File Source/bindings/core/v8/V8Binding.h (right):

https://codereview.chromium.org/993333002/diff/1/Source/bindings/core/v8/V8Bi...
Source/bindings/core/v8/V8Binding.h:439: if (value->IsUint32()) {
Now this doesn't look 'fast case'. Should we remove inline version?

Note that V8 doesn't provide Cast() for Uint32, even when IsUint32() is true. We
need to use Unit32Value(context), which could fail. Same for Int32.

https://codereview.chromium.org/990943003/#msg3

[Jens Widell, 2015-03-11 10:56:37]

https://codereview.chromium.org/993333002/diff/1/Source/bindings/core/v8/V8Bi...
File Source/bindings/core/v8/V8Binding.h (right):

https://codereview.chromium.org/993333002/diff/1/Source/bindings/core/v8/V8Bi...
Source/bindings/core/v8/V8Binding.h:439: if (value->IsUint32()) {
On 2015/03/11 10:30:37, bashi1 wrote:
> Now this doesn't look 'fast case'. Should we remove inline version?
> 
> Note that V8 doesn't provide Cast() for Uint32, even when IsUint32() is true.
We
> need to use Unit32Value(context), which could fail. Same for Int32.
> 
> https://codereview.chromium.org/990943003/#msg3

We might want to investigate the performance impact of this change as-is vs this
but uninlined. See https://codereview.chromium.org/947123002/#msg2 for some
micro-benchmarks and previous results.

But yeah, this code certainly seems to me like something we shouldn't be
inlining in lots of places.

It also seems unfortunate to me that V8 doesn't provide a fast-path for
accessing number values as int32, especially given that V8 sometimes actually
represents them directly as int32. It looks like the actual code path in
Int32Value(), in the IsNumber() case, is as "fast" as the corresponding code
path for Cast<Number> + Value().

Maybe the Number class could have accessors for int32/uint32, so that we could
do

  if (value->IsUint32())
    return value.As<v8::Number>()->Uint32Value();

?

[Yuki, 2015-03-11 13:11:52]

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

[Yuki, 2015-03-11 13:11:53]

https://codereview.chromium.org/993333002/diff/1/Source/bindings/core/v8/V8Bi...
File Source/bindings/core/v8/V8Binding.cpp (right):

https://codereview.chromium.org/993333002/diff/1/Source/bindings/core/v8/V8Bi...
Source/bindings/core/v8/V8Binding.cpp:207: if
(!value->ToNumber(context).ToLocal(&numberObject)) {
It looks to me that either of ToNumber or ToLocal should throw an exception if
it fails.  So |block.HasCaught()| should be always true.

I'd recommend to check it with V8 team.  I think V8 should define APIs in that
way.

https://codereview.chromium.org/993333002/diff/1/Source/bindings/core/v8/V8Bi...
File Source/bindings/core/v8/V8Binding.h (right):

https://codereview.chromium.org/993333002/diff/1/Source/bindings/core/v8/V8Bi...
Source/bindings/core/v8/V8Binding.h:439: if (value->IsUint32()) {
On 2015/03/11 10:56:37, Jens Widell wrote:
> On 2015/03/11 10:30:37, bashi1 wrote:
> > Now this doesn't look 'fast case'. Should we remove inline version?
> > 
> > Note that V8 doesn't provide Cast() for Uint32, even when IsUint32() is
true.
> We
> > need to use Unit32Value(context), which could fail. Same for Int32.
> > 
> > https://codereview.chromium.org/990943003/#msg3
> 
> We might want to investigate the performance impact of this change as-is vs
this
> but uninlined. See https://codereview.chromium.org/947123002/#msg2 for some
> micro-benchmarks and previous results.
> 
> But yeah, this code certainly seems to me like something we shouldn't be
> inlining in lots of places.
> 
> It also seems unfortunate to me that V8 doesn't provide a fast-path for
> accessing number values as int32, especially given that V8 sometimes actually
> represents them directly as int32. It looks like the actual code path in
> Int32Value(), in the IsNumber() case, is as "fast" as the corresponding code
> path for Cast<Number> + Value().
> 
> Maybe the Number class could have accessors for int32/uint32, so that we could
> do
> 
>   if (value->IsUint32())
>     return value.As<v8::Number>()->Uint32Value();
> 
> ?

This reminds me of the following comment on an issue:
https://code.google.com/p/chromium/issues/detail?id=446097#c7
which was fixed in the following CL:
https://codereview.chromium.org/873983003/

I'm not sure if it's possible that Uint32Value fails.  In other words, should V8
make some guarantee about IsUint32 and Uint32Value?

[bashi, 2015-03-25 08:43:22]

Patchset #2 (id:20001) has been deleted

[bashi, 2015-03-25 10:51:25]

Patchset #2 (id:40001) has been deleted

[bashi, 2015-03-25 10:54:18]

Updated. PTAL?

- Use As() when IsXXX() is true
- Use v8Call() to make sure that an exception is thrown when Maybe APIs fail.

[Jens Widell, 2015-03-25 11:11:15]

LGTM

https://codereview.chromium.org/993333002/diff/60001/Source/bindings/core/v8/...
File Source/bindings/core/v8/V8Binding.cpp (right):

https://codereview.chromium.org/993333002/diff/60001/Source/bindings/core/v8/...
Source/bindings/core/v8/V8Binding.cpp:310:
exceptionState.rethrowV8Exception(block.Exception());
Technically, this call can't fail, since numberObject is known to be a number.
It's really just performing a double->int conversion (according to the rules of
ECMA-262).

We might want to replace the error handling with some variation of assertions.

https://codereview.chromium.org/993333002/diff/60001/Source/bindings/core/v8/...
Source/bindings/core/v8/V8Binding.cpp:357:
exceptionState.rethrowV8Exception(block.Exception());
Same here.

[haraken, 2015-03-25 11:53:44]

https://codereview.chromium.org/993333002/diff/60001/Source/bindings/core/v8/...
File Source/bindings/core/v8/V8Binding.cpp (right):

https://codereview.chromium.org/993333002/diff/60001/Source/bindings/core/v8/...
Source/bindings/core/v8/V8Binding.cpp:310:
exceptionState.rethrowV8Exception(block.Exception());
On 2015/03/25 11:11:15, Jens Widell wrote:
> Technically, this call can't fail, since numberObject is known to be a number.
> It's really just performing a double->int conversion (according to the rules
of
> ECMA-262).
> 
> We might want to replace the error handling with some variation of assertions.

In practice this call won't fail, but conceptually there is no guarantee that
Int32Value() does something that can receive a worker termination signal and
throw an exception. Personally I don't want to put too much assumptions on what
V8 APIs will do, so I'd slightly prefer having the rethrowV8Exception in these
non-trivial cases. For the same reason, I might want to limit the places we use
ToLocalChecked. (ToLocalChecked means that we're assuming that the V8 API never
throws an exception, but this kind of assumption sounds a bit too implicit to
me.)

[haraken, 2015-03-25 11:53:50]

LGTM

[Jens Widell, 2015-03-25 11:57:54]

On 2015/03/25 11:53:44, haraken wrote:
>
https://codereview.chromium.org/993333002/diff/60001/Source/bindings/core/v8/...
> File Source/bindings/core/v8/V8Binding.cpp (right):
> 
>
https://codereview.chromium.org/993333002/diff/60001/Source/bindings/core/v8/...
> Source/bindings/core/v8/V8Binding.cpp:310:
> exceptionState.rethrowV8Exception(block.Exception());
> On 2015/03/25 11:11:15, Jens Widell wrote:
> > Technically, this call can't fail, since numberObject is known to be a
number.
> > It's really just performing a double->int conversion (according to the rules
> of
> > ECMA-262).
> > 
> > We might want to replace the error handling with some variation of
assertions.
> 
> In practice this call won't fail, but conceptually there is no guarantee that
> Int32Value() does something that can receive a worker termination signal and
> throw an exception. Personally I don't want to put too much assumptions on
what
> V8 APIs will do, so I'd slightly prefer having the rethrowV8Exception in these
> non-trivial cases. For the same reason, I might want to limit the places we
use
> ToLocalChecked. (ToLocalChecked means that we're assuming that the V8 API
never
> throws an exception, but this kind of assumption sounds a bit too implicit to
> me.)

Makes sense.

I guess if we wanted to optimize this (which in this case seems quite
unnecessary) a reasonable approach would perhaps be to single out the
double->int32 conversion into a utility API (that really, really can't fail).

[Yuki, 2015-03-25 12:39:37]

lgtm

[bashi, 2015-03-26 01:30:49]

Thanks for reviews and comments. Let me land this:)

[bashi, 2015-03-26 01:30:54]

The CQ bit was checked by bashi@chromium.org

[commit-bot: I haz the power, 2015-03-26 01:31:34]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/993333002/60001

[commit-bot: I haz the power, 2015-03-26 01:35:37]

Committed patchset #2 (id:60001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=192568

[Yuki, 2016-04-28 06:05:48]

Description was changed from

==========
bindings: Use Maybe APIs in toXXX() functions

BUG=462402

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=192568
==========

to

==========
bindings: Use Maybe APIs in toXXX() functions

BUG=462402

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=192568
==========

[Yuki, 2016-04-28 06:05:48]

yukishiino@chromium.org changed reviewers:
- yukishiino@chromium.or