Issue 947123002: [bindings] Split toUInt32 into faster inline and non-inline slower path.

Issue 947123002: [bindings] Split toUInt32 into faster inline and non-inline slower path. (Closed)

Created:
5 years, 10 months ago by vivekg_samsung

Modified:
5 years, 10 months ago

Reviewers:
haraken, vivekg, Jens Widell

CC:
arv+blink, blink-reviews, blink-reviews-bindings_chromium.org, vivekg_samsung

Base URL:
https://chromium.googlesource.com/chromium/blink.git@master

Target Ref:
refs/heads/master

Project:
blink

Visibility:
Public.

More Reviews

Description

[bindings] Split toUInt32 into faster inline and non-inline slower path. R=haraken@chromium.org, jl@opera.com Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=190741

Patch Set 2 : Typo correction! #

Patch Set 3 : 'or' condition should be 'and' #

Patch Set 4 : Review comments addressed! #

Patch Set 6 : Patch for landing #

Created: 5 years, 10 months ago

Download [raw] [tar.bz2]

		Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+26 lines, -22 lines)			Patch
	M	Source/bindings/core/v8/V8Binding.h	View	1 2 3	1 chunk	+14 lines, -1 line	0 comments	Download
	M	Source/bindings/core/v8/V8Binding.cpp	View	1 2 3 4 5	2 chunks	+12 lines, -21 lines	0 comments	Download

Messages

Total messages: 14 (4 generated)

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages

vivekg

PTAL, here are the stats. micro-benchmark-unsigned.html ============================= <!DOCTYPE html> <body> <script src="../resources/runner.js"></script> <script> PerfTestRunner.measureTime({ run: ...

5 years, 10 months ago (2015-02-23 11:29:07 UTC) #2

PTAL, here are the stats.

micro-benchmark-unsigned.html
=============================
<!DOCTYPE html>
<body>
<script src="../resources/runner.js"></script>
<script>
PerfTestRunner.measureTime({
    run: function() {
        (function() {
            var x = new XMLHttpRequest();
            for (var i = 0; i < 1000000; i++)
                x.timeout = i % 1000;
        })();
    }
});
</script>
</body>

micro-benchmark-signed.html
===========================
<!DOCTYPE html>
<body>
<script src="../resources/runner.js"></script>
<script>
PerfTestRunner.measureTime({
    run: function() {
        (function() {
            var x = new XMLHttpRequest();
            for (var i = 0; i < 1000000; i++)
                x.timeout = -1 * (i % 1000);
        })();
    }
});
</script>
</body>

micro-benchmark-non-number.html
===============================
<!DOCTYPE html>
<body>
<script src="../resources/runner.js"></script>
<script>
PerfTestRunner.measureTime({
    run: function() {
        (function() {
            var x = new XMLHttpRequest();
            for (var i = 0; i < 1000000; i++)
                x.timeout = true;           
        })();
    }
});
</script>
</body>


+-------------------------------------------+----------+----------+---------+
| Test - ANDROID                            | Unit     | Without  |  With   |
+-------------------------------------------+----------+----------+---------+
| blink_perf.bindings: micro-benchmark-unsigned.html                        |
+-------------------------------------------+----------+----------+---------+
|                                 Run 1     | ms       | 246.1    | 230.1   |
|                                 Run 2     | ms       | 252.6    | 243.8   |
|                                 Run 3     | ms       | 234.7    | 260.1   |
|                                 Run 4     | ms       | 246.5    | 242.7   |
|                                 Run 5     | ms       | 253.8    | 238.6   |
+-------------------------------------------+----------+----------+---------+
|              Average                      | ms       | 246.7    | 243.1   |
+-------------------------------------------+----------+----------+---------+
| blink_perf.bindings: micro-benchmark-signed.html                          |
+-------------------------------------------+----------+----------+---------+
|                                 Run 1     | ms       | 336.7    | 314.4   |
|                                 Run 2     | ms       | 359.1    | 335.0   |
|                                 Run 3     | ms       | 352.5    | 348.8   |
|                                 Run 4     | ms       | 358.1    | 351.5   |
|                                 Run 5     | ms       | 342.7    | 344.6   |
+-------------------------------------------+----------+----------+---------+
|              Average                      | ms       | 349.8    | 338.9   |
+-------------------------------------------+----------+----------+---------+
| blink_perf.bindings: micro-benchmark-non-number.html                      |
+-------------------------------------------+----------+----------+---------+
|                                 Run 1     | ms       | 932.4    | 833.2   |
|                                 Run 2     | ms       | 923.9    | 911.7   |
|                                 Run 3     | ms       | 912.9    | 914.7   |
|                                 Run 4     | ms       | 921.7    | 925.5   |
|                                 Run 5     | ms       | 913.5    | 920.6   |
+-------------------------------------------+----------+----------+---------+
|              Average                      | ms       | 920.9    | 901.1   |
+-------------------------------------------+----------+----------+---------+

Jens Widell

https://codereview.chromium.org/947123002/diff/1/Source/bindings/core/v8/V8Binding.h File Source/bindings/core/v8/V8Binding.h (right): https://codereview.chromium.org/947123002/diff/1/Source/bindings/core/v8/V8Binding.h#newcode447 Source/bindings/core/v8/V8Binding.h:447: if (configuration == EnforceRange) { I think in principle ...

5 years, 10 months ago (2015-02-23 11:41:57 UTC) #5

vivekg

On 2015/02/23 at 11:41:57, jl wrote: > https://codereview.chromium.org/947123002/diff/1/Source/bindings/core/v8/V8Binding.h > File Source/bindings/core/v8/V8Binding.h (right): > > https://codereview.chromium.org/947123002/diff/1/Source/bindings/core/v8/V8Binding.h#newcode447 ...

5 years, 10 months ago (2015-02-23 13:42:12 UTC) #6

Jens Widell

Sorry about the delay. LGTM. https://codereview.chromium.org/947123002/diff/80001/Source/bindings/core/v8/V8Binding.cpp File Source/bindings/core/v8/V8Binding.cpp (right): https://codereview.chromium.org/947123002/diff/80001/Source/bindings/core/v8/V8Binding.cpp#newcode365 Source/bindings/core/v8/V8Binding.cpp:365: } else { No ...

5 years, 10 months ago (2015-02-24 07:43:09 UTC) #8

vivekg

On 2015/02/24 at 07:43:09, jl wrote: > Sorry about the delay. LGTM. > > https://codereview.chromium.org/947123002/diff/80001/Source/bindings/core/v8/V8Binding.cpp ...

5 years, 10 months ago (2015-02-24 07:55:51 UTC) #9

commit-bot: I haz the power

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

5 years, 10 months ago (2015-02-24 09:09:15 UTC) #13

commit-bot: I haz the power

5 years, 10 months ago (2015-02-24 10:43:47 UTC) #14

Message was sent while issue was closed.

Committed patchset #6 (id:100001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=190741

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages