Reland: Use simple/fast inline function version of MinMax in JS

Reland: Use simple/fast inline function version of MinMax in JS

Use the simple inline function version of {Min, Max} where possible to
improve performance

Now uses an forced inline js function instead of a python macro
to avoid expressions be evaluated twice

Follow-up to CR: https://codereview.chromium.org/1331993004

Committed: https://crrev.com/b00371b423f52353a4f31e179cc4be7bd2cde973
Cr-Commit-Position: refs/heads/master@{#31411}

[skomski, 2015-10-15 12:55:18]

karl@skomski.com changed reviewers:
+ jkummerow@chromium.org, littledan@chromium.org

[skomski, 2015-10-15 12:55:19]

PTAL
Reland with inline function instead of macro.

Currently the test is not using the internal function.
How can I expose an internal function in the test without adding it to the
natives object like $sameValue?

[Dan Ehrenberg, 2015-10-15 12:59:45]

On 2015/10/15 at 12:55:19, karl wrote:
> PTAL
> Reland with inline function instead of macro.
> 
> Currently the test is not using the internal function.
> How can I expose an internal function in the test without adding it to the
natives object like $sameValue?

Probably you should git pull --rebase and mail this again; you seem to have not
included my revert of your last patch.

[skomski, 2015-10-15 13:27:37]

On 2015/10/15 12:59:45, Dan Ehrenberg wrote:
> On 2015/10/15 at 12:55:19, karl wrote:
> > PTAL
> > Reland with inline function instead of macro.
> > 
> > Currently the test is not using the internal function.
> > How can I expose an internal function in the test without adding it to the
> natives object like $sameValue?
> 
> Probably you should git pull --rebase and mail this again; you seem to have
not
> included my revert of your last patch.

Rebased.

[Jakob Kummerow, 2015-10-15 14:18:48]

https://codereview.chromium.org/1410473002/diff/60001/src/js/array.js
File src/js/array.js (right):

https://codereview.chromium.org/1410473002/diff/60001/src/js/array.js#newcode26
src/js/array.js:26: var MinSimple;
nit: please preserve alpha-sorting

https://codereview.chromium.org/1410473002/diff/60001/src/js/arraybuffer.js
File src/js/arraybuffer.js (right):

https://codereview.chromium.org/1410473002/diff/60001/src/js/arraybuffer.js#n...
src/js/arraybuffer.js:18: var MaxSimple;
nit: please preserve alpha-sorting

https://codereview.chromium.org/1410473002/diff/60001/src/js/harmony-array.js
File src/js/harmony-array.js (right):

https://codereview.chromium.org/1410473002/diff/60001/src/js/harmony-array.js...
src/js/harmony-array.js:20: var MaxSimple;
nit: please preserve alpha-sorting

https://codereview.chromium.org/1410473002/diff/60001/src/js/harmony-atomics.js
File src/js/harmony-atomics.js (right):

https://codereview.chromium.org/1410473002/diff/60001/src/js/harmony-atomics....
src/js/harmony-atomics.js:16: var MaxSimple;
nit: please preserve alpha-sorting

https://codereview.chromium.org/1410473002/diff/60001/src/js/harmony-typedarr...
File src/js/harmony-typedarray.js (right):

https://codereview.chromium.org/1410473002/diff/60001/src/js/harmony-typedarr...
src/js/harmony-typedarray.js:52: var MaxSimple;
nit: please preserve alpha-sorting

https://codereview.chromium.org/1410473002/diff/60001/src/js/json.js
File src/js/json.js (right):

https://codereview.chromium.org/1410473002/diff/60001/src/js/json.js#newcode18
src/js/json.js:18: var MaxSimple;
nit: please preserve alpha-sorting

https://codereview.chromium.org/1410473002/diff/60001/src/js/runtime.js
File src/js/runtime.js (right):

https://codereview.chromium.org/1410473002/diff/60001/src/js/runtime.js#newco...
src/js/runtime.js:234: %SetForceInlineFlag(MaxSimple);
Have you measured that this actually makes a difference, or are you just
guessing?

https://codereview.chromium.org/1410473002/diff/60001/test/mjsunit/minmax-sim...
File test/mjsunit/minmax-simple.js (right):

https://codereview.chromium.org/1410473002/diff/60001/test/mjsunit/minmax-sim...
test/mjsunit/minmax-simple.js:36: assertEquals(2, checkEvaluations(MaxSimple));
Clearly you mean MinSimple here.

[skomski, 2015-10-15 15:17:59]

addressed comments by jakob. Still solution needed for test. ToPositiveInteger
with the same problem unfortunately lacks any test.

https://codereview.chromium.org/1410473002/diff/60001/src/js/array.js
File src/js/array.js (right):

https://codereview.chromium.org/1410473002/diff/60001/src/js/array.js#newcode26
src/js/array.js:26: var MinSimple;
On 2015/10/15 14:18:48, Jakob wrote:
> nit: please preserve alpha-sorting

Done.

https://codereview.chromium.org/1410473002/diff/60001/src/js/arraybuffer.js
File src/js/arraybuffer.js (right):

https://codereview.chromium.org/1410473002/diff/60001/src/js/arraybuffer.js#n...
src/js/arraybuffer.js:18: var MaxSimple;
On 2015/10/15 14:18:48, Jakob wrote:
> nit: please preserve alpha-sorting

Done.

https://codereview.chromium.org/1410473002/diff/60001/src/js/harmony-array.js
File src/js/harmony-array.js (right):

https://codereview.chromium.org/1410473002/diff/60001/src/js/harmony-array.js...
src/js/harmony-array.js:20: var MaxSimple;
On 2015/10/15 14:18:48, Jakob wrote:
> nit: please preserve alpha-sorting

Done.

https://codereview.chromium.org/1410473002/diff/60001/src/js/harmony-atomics.js
File src/js/harmony-atomics.js (right):

https://codereview.chromium.org/1410473002/diff/60001/src/js/harmony-atomics....
src/js/harmony-atomics.js:16: var MaxSimple;
On 2015/10/15 14:18:48, Jakob wrote:
> nit: please preserve alpha-sorting

Done.

https://codereview.chromium.org/1410473002/diff/60001/src/js/harmony-typedarr...
File src/js/harmony-typedarray.js (right):

https://codereview.chromium.org/1410473002/diff/60001/src/js/harmony-typedarr...
src/js/harmony-typedarray.js:52: var MaxSimple;
On 2015/10/15 14:18:48, Jakob wrote:
> nit: please preserve alpha-sorting

Done.

https://codereview.chromium.org/1410473002/diff/60001/src/js/json.js
File src/js/json.js (right):

https://codereview.chromium.org/1410473002/diff/60001/src/js/json.js#newcode18
src/js/json.js:18: var MaxSimple;
On 2015/10/15 14:18:48, Jakob wrote:
> nit: please preserve alpha-sorting

Done.

https://codereview.chromium.org/1410473002/diff/60001/src/js/runtime.js
File src/js/runtime.js (right):

https://codereview.chromium.org/1410473002/diff/60001/src/js/runtime.js#newco...
src/js/runtime.js:234: %SetForceInlineFlag(MaxSimple);
On 2015/10/15 14:18:48, Jakob wrote:
> Have you measured that this actually makes a difference, or are you just
> guessing?

Measured and definitely makes a big difference >9%.

https://codereview.chromium.org/1410473002/diff/60001/test/mjsunit/minmax-sim...
File test/mjsunit/minmax-simple.js (right):

https://codereview.chromium.org/1410473002/diff/60001/test/mjsunit/minmax-sim...
test/mjsunit/minmax-simple.js:36: assertEquals(2, checkEvaluations(MaxSimple));
On 2015/10/15 14:18:48, Jakob wrote:
> Clearly you mean MinSimple here.

Done.

[Jakob Kummerow, 2015-10-15 17:39:24]

On 2015/10/15 15:17:59, skomski wrote:
> addressed comments by jakob. Still solution needed for test.

AFAIK we currently don't have a good way to unit-test such helper functions. (We
used to have one, --expose-builtins-as, but it fell victim to restructurings. We
haven't come up with a replacement yet.)

So one option is to keep the test you have.

On the other hand, the purpose of checking in tests that run on every commit is
to notice when stuff breaks. This test, being a copy of the production code,
won't notice anything. So it can be argued that it has served its purpose
(convincing you and us that the functions are currently working correctly), and
can be omitted from the commit.

[skomski, 2015-10-20 12:33:51]

On 2015/10/15 17:39:24, Jakob wrote:
> On 2015/10/15 15:17:59, skomski wrote:
> > addressed comments by jakob. Still solution needed for test.
> 
> AFAIK we currently don't have a good way to unit-test such helper functions.
(We
> used to have one, --expose-builtins-as, but it fell victim to restructurings.
We
> haven't come up with a replacement yet.)
> 
> So one option is to keep the test you have.
> 
> On the other hand, the purpose of checking in tests that run on every commit
is
> to notice when stuff breaks. This test, being a copy of the production code,
> won't notice anything. So it can be argued that it has served its purpose
> (convincing you and us that the functions are currently working correctly),
and
> can be omitted from the commit.

Ping. Now possible to check production code in test.

[Dan Ehrenberg, 2015-10-20 13:22:33]

lgtm

[Dan Ehrenberg, 2015-10-20 13:22:42]

The CQ bit was checked by littledan@chromium.org

[commit-bot: I haz the power, 2015-10-20 13:22:46]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1410473002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1410473002/120001

[commit-bot: I haz the power, 2015-10-20 13:43:49]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-10-20 13:44:07]

Patchset 7 (id:??) landed as
https://crrev.com/b00371b423f52353a4f31e179cc4be7bd2cde973
Cr-Commit-Position: refs/heads/master@{#31411}