Migrate Math.Min/Max to CodeStubAssembler

Migrate Math.Min/Max to CodeStubAssembler

BUG=v8:6026
Review-Url: https://codereview.chromium.org/2728463006
Cr-Commit-Position: refs/heads/master@{#43600}
Committed: https://chromium.googlesource.com/v8/v8/+/49ff8c1d2044b640c5907b91093cb0866bb23647

[vabr (Chromium), 2017-03-03 10:27:29]

vabr@chromium.org changed reviewers:
+ bmeurer@chromium.org

[vabr (Chromium), 2017-03-03 10:27:30]

Hi Benedikt,
I uploaded the first version here so that we can discuss it. It only migrates
max so far and it is not deleting the platform code yet.
"tools/run-tests.py --gn mjsunit/math-min-max" passes, though.

More importantly, it is significantly slower. My benchmark is at
https://crbug.com/v8/6026#c1.
 * Before my patch it consistently showed about 430ms for both max and min.
 * After my patch, the min increased to about 480ms (surprise, because it was
not changed by the patch), and the max even to 570ms.


It seems like I'll need to try to keep the tagged and untagged versions of the
numbers in parallel, unless you see another potential inefficiency in the code.

Cheers,
Vaclav

[vabr (Chromium), 2017-03-03 10:39:14]

https://codereview.chromium.org/2728463006/diff/1/src/builtins/builtins-math.cc
File src/builtins/builtins-math.cc (right):

https://codereview.chromium.org/2728463006/diff/1/src/builtins/builtins-math....
src/builtins/builtins-math.cc:520: GotoIf(Float64Equal(float_value, nan),
&return_nan);
Interestingly, when I remove this jump, the performance of max drops to about
490ms, similar to the 480ms of min.
(Still no idea why even the min jumped up from 430ms after this change.)

I need to follow the spec on returning NaN if some of the numbers are NaN, but
that can be arranged without the jump if I'm fine with finishing the whole loop
in all cases.

[vabr (Chromium), 2017-03-03 12:18:38]

With the updated benchmark https://crbug.com/v8/6026#c2 and the patch set 2 I
observed no change for min (expected), and an increase for max from about 490ms
before the change to about 510ms after it.

Where do we draw the line, Benedikt, is that increase too high?

Cheers,
Vaclav

[vabr (Chromium), 2017-03-03 13:03:31]

The CQ bit was checked by vabr@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-03 13:03:34]

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

[vabr (Chromium), 2017-03-03 13:19:19]

Hi Benedikt.

The current patch set 4 is ready for review.

I updated the benchmark so that the min and max are non-SMIs (see
https://crbug.com/v8/6026#c3). With that the results are:

Before CL: min ~570ms, max ~560ms.
After CL: min ~570ms, max ~590ms.
So min stayed the same and max increased a little. Given the noise between
different trials (about a 100ms between fastest and slowest runs with the same
code), these differences do not seem very significant.

Cheers,
Vaclav

[commit-bot: I haz the power, 2017-03-03 13:20:58]

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

[commit-bot: I haz the power, 2017-03-03 13:21:01]

Dry run: Try jobs failed on following builders:
  v8_win_nosnap_shared_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win_nosnap_shared_rel_ng...)
  v8_win_nosnap_shared_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win_nosnap_shared_rel_ng...)

[vabr (Chromium), 2017-03-03 13:30:31]

The CQ bit was checked by vabr@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-03 13:30:35]

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-03-03 13:59:29]

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

[commit-bot: I haz the power, 2017-03-03 13:59:29]

Dry run: This issue passed the CQ dry run.

[Benedikt Meurer, 2017-03-03 17:58:04]

Description was changed from

==========
Migrate Math.Min/Max to CodeStubAssembler

BUG=6026
==========

to

==========
Migrate Math.Min/Max to CodeStubAssembler

BUG=v8:6026
==========

[Benedikt Meurer, 2017-03-03 18:02:12]

Woohoo! Very nice, saving 1000 lines of handwritten native code, and replacing
it with a very tight and understandable platform independent solution with
roughly similar performance.

That's definitely LGTM! Thanks a lot Vaclav.

[vabr (Chromium), 2017-03-04 13:51:42]

On 2017/03/03 18:02:12, Benedikt Meurer wrote:
> Woohoo! Very nice, saving 1000 lines of handwritten native code, and replacing
> it with a very tight and understandable platform independent solution with
> roughly similar performance.
> 
> That's definitely LGTM! Thanks a lot Vaclav.

Thanks a lot for the patience and guidance, Benedikt! :)

[vabr (Chromium), 2017-03-04 13:52:14]

On 2017/03/04 13:51:42, vabr (Chromium) wrote:
> On 2017/03/03 18:02:12, Benedikt Meurer wrote:
> > Woohoo! Very nice, saving 1000 lines of handwritten native code, and
replacing
> > it with a very tight and understandable platform independent solution with
> > roughly similar performance.
> > 
> > That's definitely LGTM! Thanks a lot Vaclav.
> 
> Thanks a lot for the patience and guidance, Benedikt! :)

...and for fixing the bug reference!

[vabr (Chromium), 2017-03-04 13:52:21]

The CQ bit was checked by vabr@chromium.org

[commit-bot: I haz the power, 2017-03-04 13:52:23]

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-03-04 14:22:15]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1488635541452670,
"parent_rev": "fcce4797bc678958d527f93b46d2de2bdbf31dcc", "commit_rev":
"49ff8c1d2044b640c5907b91093cb0866bb23647"}

[commit-bot: I haz the power, 2017-03-04 14:22:20]

Description was changed from

==========
Migrate Math.Min/Max to CodeStubAssembler

BUG=v8:6026
==========

to

==========
Migrate Math.Min/Max to CodeStubAssembler

BUG=v8:6026

Review-Url: https://codereview.chromium.org/2728463006
Cr-Commit-Position: refs/heads/master@{#43600}
Committed:
https://chromium.googlesource.com/v8/v8/+/49ff8c1d2044b640c5907b91093cb0866bb...
==========

[commit-bot: I haz the power, 2017-03-04 14:22:21]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/v8/v8/+/49ff8c1d2044b640c5907b91093cb0866bb...