bindings: Add performance test for JS->WebIDL sequence conversions.

bindings: Add performance test for JS->WebIDL sequence conversions.

At the moment, we only test it with arrays, as we are still working on the
code to convert arbitrary objects with an @@iterator property into
sequences.

BUG=685754
R=bashi@chromium.org,haraken@chromium.org,yukishiino@chromium.org

Review-Url: https://codereview.chromium.org/2821493002
Cr-Commit-Position: refs/heads/master@{#465207}
Committed: https://chromium.googlesource.com/chromium/src/+/d54885458b5fbe0ea6a3d9997457cef8e12243dd

[Raphael Kubo da Costa (rakuco), 2017-04-13 17:34:43]

The CQ bit was checked by raphael.kubo.da.costa@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-04-13 17:35:16]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Raphael Kubo da Costa (rakuco), 2017-04-13 17:35:54]

PTAL.

I also have another test for non-arrays, but it needs to land with
https://codereview.chromium.org/2810843002 as Blink currently cannot convert
them into sequences.

I'd prefer to have used window.internals in the test, but it looks like it is
not available for the performance tests, so I used Blob instead.

[commit-bot: I haz the power, 2017-04-13 19:10:07]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-13 19:10:08]

Dry run: This issue passed the CQ dry run.

[haraken, 2017-04-13 23:00:47]

On 2017/04/13 17:35:54, Raphael Kubo da Costa (rakuco) wrote:
> PTAL.
> 
> I also have another test for non-arrays, but it needs to land with
> https://codereview.chromium.org/2810843002 as Blink currently cannot convert
> them into sequences.
> 
> I'd prefer to have used window.internals in the test, but it looks like it is
> not available for the performance tests, so I used Blob instead.

Yeah, the performance tests need to run on other browsers, so we don't want to
expose window.internals.

Just to confirm: The performance of the JS => IDL sequence conversion is
dominant in the Blob instantiation, right?

[bashi, 2017-04-14 00:19:20]

lgtm but please wait for haraken@'s approval.

On 2017/04/13 23:00:47, haraken wrote:
> On 2017/04/13 17:35:54, Raphael Kubo da Costa (rakuco) wrote:
> > PTAL.
> > 
> > I also have another test for non-arrays, but it needs to land with
> > https://codereview.chromium.org/2810843002 as Blink currently cannot convert
> > them into sequences.
> > 
> > I'd prefer to have used window.internals in the test, but it looks like it
is
> > not available for the performance tests, so I used Blob instead.
> 
> Yeah, the performance tests need to run on other browsers, so we don't want to
> expose window.internals.
> 
> Just to confirm: The performance of the JS => IDL sequence conversion is
> dominant in the Blob instantiation, right?

In https://codereview.chromium.org/2810843002/#msg9 Raphael said that ~25k
runs/second (array) vs ~8k runs/second (iterator) so it's not the case?

[Yuki, 2017-04-14 07:50:44]

LGTM.

https://codereview.chromium.org/2821493002/diff/1/third_party/WebKit/Performa...
File third_party/WebKit/PerformanceTests/Bindings/sequence-conversion-array.html
(right):

https://codereview.chromium.org/2821493002/diff/1/third_party/WebKit/Performa...
third_party/WebKit/PerformanceTests/Bindings/sequence-conversion-array.html:8:
dataArray.push('abcdefg foo bar baz');
Will this v8::String be converted to blink::String or something?

If we'd like to measure the pure overhead of v8::Array => IDLSequence, then we
wouldn't be interested in the content nor content's conversion.  Maybe simple
zero or boolean would be better?

[Raphael Kubo da Costa (rakuco), 2017-04-14 08:09:01]

On 2017/04/14 00:19:20, bashi wrote:
> On 2017/04/13 23:00:47, haraken wrote:
> > Just to confirm: The performance of the JS => IDL sequence conversion is
> > dominant in the Blob instantiation, right?
> 
> In https://codereview.chromium.org/2810843002/#msg9 Raphael said that ~25k
> runs/second (array) vs ~8k runs/second (iterator) so it's not the case?

Could you clarify the best way to determine that? Blob::Create() does some
additional work that involves iterating over the generated HeapVector later,
appending data to internal data structures etc. It also has the overhead of
dealing with a union, so each array iteration in the conversion also checks if a
given element is of a type other than string. There aren't many options though
-- I originally thought of using a Headers instead of a Blob since the
additional work done in Headers::Create() is smaller, and could go back to that
one.

[Raphael Kubo da Costa (rakuco), 2017-04-14 08:09:10]

https://codereview.chromium.org/2821493002/diff/1/third_party/WebKit/Performa...
File third_party/WebKit/PerformanceTests/Bindings/sequence-conversion-array.html
(right):

https://codereview.chromium.org/2821493002/diff/1/third_party/WebKit/Performa...
third_party/WebKit/PerformanceTests/Bindings/sequence-conversion-array.html:8:
dataArray.push('abcdefg foo bar baz');
On 2017/04/14 07:50:44, Yuki wrote:
> Will this v8::String be converted to blink::String or something?
> 
> If we'd like to measure the pure overhead of v8::Array => IDLSequence, then we
> wouldn't be interested in the content nor content's conversion.  Maybe simple
> zero or boolean would be better?

Since the string conversion is still going to happen due to Blob's constructor
not taking numbers or booleans, how about an empty string instead?

[Yuki, 2017-04-14 08:27:58]

https://codereview.chromium.org/2821493002/diff/1/third_party/WebKit/Performa...
File third_party/WebKit/PerformanceTests/Bindings/sequence-conversion-array.html
(right):

https://codereview.chromium.org/2821493002/diff/1/third_party/WebKit/Performa...
third_party/WebKit/PerformanceTests/Bindings/sequence-conversion-array.html:8:
dataArray.push('abcdefg foo bar baz');
On 2017/04/14 08:09:10, Raphael Kubo da Costa (rakuco) wrote:
> On 2017/04/14 07:50:44, Yuki wrote:
> > Will this v8::String be converted to blink::String or something?
> > 
> > If we'd like to measure the pure overhead of v8::Array => IDLSequence, then
we
> > wouldn't be interested in the content nor content's conversion.  Maybe
simple
> > zero or boolean would be better?
> 
> Since the string conversion is still going to happen due to Blob's constructor
> not taking numbers or booleans, how about an empty string instead?

An empty string makes sense to me.

[Raphael Kubo da Costa (rakuco), 2017-04-14 10:58:57]

On 2017/04/14 08:27:58, Yuki wrote:
> An empty string makes sense to me.

Done. This also makes the numbers go up -- my latest average is
70.79481642105483 runs/s.

[Raphael Kubo da Costa (rakuco), 2017-04-14 10:59:05]

The CQ bit was checked by raphael.kubo.da.costa@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-04-14 10:59:11]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-14 11:59:30]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-14 11:59:30]

Dry run: This issue passed the CQ dry run.

[haraken, 2017-04-14 19:03:35]

On 2017/04/14 10:58:57, rakuco (OoO on the 17th) wrote:
> On 2017/04/14 08:27:58, Yuki wrote:
> > An empty string makes sense to me.
> 
> Done. This also makes the numbers go up -- my latest average is
> 70.79481642105483 runs/s.

LGTM

If Headers does less additional work than Blob, I'd prefer Headers though.

[Raphael Kubo da Costa (rakuco), 2017-04-18 10:52:00]

Hmm, I tried using Headers here but the numbers went from 70k/second to
~30k/second, most likely because with Headers we need to perform additional
work: dataArray.push('') needs to become dataArray.push(['x', '']), for example.
I think I'll land the Blob version.

[Raphael Kubo da Costa (rakuco), 2017-04-18 10:52:06]

The CQ bit was checked by raphael.kubo.da.costa@intel.com

[Raphael Kubo da Costa (rakuco), 2017-04-18 10:52:06]

The patchset sent to the CQ was uploaded after l-g-t-m from bashi@chromium.org,
yukishiino@chromium.org
Link to the patchset: https://codereview.chromium.org/2821493002/#ps20001
(title: "Push empty strings into the array")

[commit-bot: I haz the power, 2017-04-18 10:52:22]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-18 12:19:12]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1492512726454370,
"parent_rev": "5047940999a9cf13e8635e61112fdaa7db885726", "commit_rev":
"d54885458b5fbe0ea6a3d9997457cef8e12243dd"}

[commit-bot: I haz the power, 2017-04-18 12:19:22]

Description was changed from

==========
bindings: Add performance test for JS->WebIDL sequence conversions.

At the moment, we only test it with arrays, as we are still working on the
code to convert arbitrary objects with an @@iterator property into
sequences.

BUG=685754
R=bashi@chromium.org,haraken@chromium.org,yukishiino@chromium.org
==========

to

==========
bindings: Add performance test for JS->WebIDL sequence conversions.

At the moment, we only test it with arrays, as we are still working on the
code to convert arbitrary objects with an @@iterator property into
sequences.

BUG=685754
R=bashi@chromium.org,haraken@chromium.org,yukishiino@chromium.org

Review-Url: https://codereview.chromium.org/2821493002
Cr-Commit-Position: refs/heads/master@{#465207}
Committed:
https://chromium.googlesource.com/chromium/src/+/d54885458b5fbe0ea6a3d9997457...
==========

[commit-bot: I haz the power, 2017-04-18 12:19:23]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/d54885458b5fbe0ea6a3d9997457...