[SAB] Move Atomics builtins to C++

[SAB] Move Atomics builtins to C++

This patch refactors the Atomics builtins so that they are implemented
as C++ builtins rather than experimental JS builtins. Previously, each
of these functions called out to a runtime function, so no significant
change in performance is anticipated. The goal of this patch is to
remove the last user of experimental JS builtins so that the mechanism
can be removed, for performance reasons. The patch includes a drive-by
fix of a check-fail. For the most part, the patch is just moving code
without modification from runtime-atomics.cc to
builtins-sharedarraybuffer.cc .

BUG=v8:5880
Review-Url: https://codereview.chromium.org/2698813004
Cr-Commit-Position: refs/heads/master@{#43335}
Committed: https://chromium.googlesource.com/v8/v8/+/2b9840d86f3b51f42918b6bf1a06ff8b62b2464d

[Dan Ehrenberg, 2017-02-16 17:41:45]

Description was changed from

==========
Staring to port atomics to C++

BUG=
==========

to

==========
[SAB] Move Atomics builtins to C++

This patch refactors the Atomics builtins so that they are implemented
as C++ builtins rather than experimental JS builtins. Previously, each
of these functions called out to a runtime function, so no significant
change in performance is anticipated. The goal of this patch is to
remove the last user of experimental JS builtins so that the mechanism
can be removed, for performance reasons.

BUG=v8:5880
==========

[Dan Ehrenberg, 2017-02-16 17:41:45]

littledan@chromium.org changed reviewers:
+ binji@chromium.org

[Dan Ehrenberg, 2017-02-16 17:42:00]

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

[commit-bot: I haz the power, 2017-02-16 17:42:09]

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

[Dan Ehrenberg, 2017-02-16 17:44:39]

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

[commit-bot: I haz the power, 2017-02-16 17:44:51]

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

[Dan Ehrenberg, 2017-02-16 17:45:37]

Description was changed from

==========
[SAB] Move Atomics builtins to C++

This patch refactors the Atomics builtins so that they are implemented
as C++ builtins rather than experimental JS builtins. Previously, each
of these functions called out to a runtime function, so no significant
change in performance is anticipated. The goal of this patch is to
remove the last user of experimental JS builtins so that the mechanism
can be removed, for performance reasons.

BUG=v8:5880
==========

to

==========
[SAB] Move Atomics builtins to C++

This patch refactors the Atomics builtins so that they are implemented
as C++ builtins rather than experimental JS builtins. Previously, each
of these functions called out to a runtime function, so no significant
change in performance is anticipated. The goal of this patch is to
remove the last user of experimental JS builtins so that the mechanism
can be removed, for performance reasons. The patch includes a drive-by
fix of a check-fail. For the most part, the patch is just moving code
without modification from runtime-atomics.cc to
builtins-sharedarraybuffer.cc .

BUG=v8:5880
==========

[commit-bot: I haz the power, 2017-02-16 17:47:59]

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

[commit-bot: I haz the power, 2017-02-16 17:48:00]

Dry run: Try jobs failed on following builders:
  v8_win64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win64_rel_ng/builds/22743)
  v8_win_compile_dbg on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win_compile_dbg/builds/3...)
  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...)

[Dan Ehrenberg, 2017-02-16 17:56:50]

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

[commit-bot: I haz the power, 2017-02-16 17:56:59]

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-02-16 18:24:41]

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

[commit-bot: I haz the power, 2017-02-16 18:24:42]

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...)

[Dan Ehrenberg, 2017-02-17 08:27:50]

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

[commit-bot: I haz the power, 2017-02-17 08:27:52]

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-02-17 08:55:31]

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

[commit-bot: I haz the power, 2017-02-17 08:55:32]

Dry run: This issue passed the CQ dry run.

[Dan Ehrenberg, 2017-02-17 09:16:07]

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

[commit-bot: I haz the power, 2017-02-17 09:16: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-02-17 09:41:01]

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

[commit-bot: I haz the power, 2017-02-17 09:41:02]

Dry run: This issue passed the CQ dry run.

[binji, 2017-02-17 19:44:41]

lgtm, thanks for doing this!

https://codereview.chromium.org/2698813004/diff/110001/src/builtins/builtins-...
File src/builtins/builtins-sharedarraybuffer.cc (right):

https://codereview.chromium.org/2698813004/diff/110001/src/builtins/builtins-...
src/builtins/builtins-sharedarraybuffer.cc:290: Isolate* isolate, Handle<Object>
object, bool onlyInt32 = false) {
nit: hacker_case for variables... or is the idea to keep the names matching the
ES spec?

https://codereview.chromium.org/2698813004/diff/110001/src/builtins/builtins-...
src/builtins/builtins-sharedarraybuffer.cc:343: Handle<Object> array =
args.atOrUndefined(isolate, 1);
Can these just be at, since they're not optional? I don't really understand the
difference here. :)

https://codereview.chromium.org/2698813004/diff/110001/src/builtins/builtins-...
src/builtins/builtins-sharedarraybuffer.cc:641: // Uint8Clamped functions
Oops, the Uint8Clamped versions should have been removed. Might as well do it
now!

[Dan Ehrenberg, 2017-02-20 21:34:11]

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

[commit-bot: I haz the power, 2017-02-20 21:34:17]

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

[Dan Ehrenberg, 2017-02-20 21:34:21]

https://codereview.chromium.org/2698813004/diff/110001/src/builtins/builtins-...
File src/builtins/builtins-sharedarraybuffer.cc (right):

https://codereview.chromium.org/2698813004/diff/110001/src/builtins/builtins-...
src/builtins/builtins-sharedarraybuffer.cc:290: Isolate* isolate, Handle<Object>
object, bool onlyInt32 = false) {
On 2017/02/17 19:44:41, binji wrote:
> nit: hacker_case for variables... or is the idea to keep the names matching
the
> ES spec?

You're right, fixed.

https://codereview.chromium.org/2698813004/diff/110001/src/builtins/builtins-...
src/builtins/builtins-sharedarraybuffer.cc:343: Handle<Object> array =
args.atOrUndefined(isolate, 1);
On 2017/02/17 19:44:41, binji wrote:
> Can these just be at, since they're not optional? I don't really understand
the
> difference here. :)

It looks like that would make a DCHECK fail if it's invoked with zero arguments,
on line 37 of src/builtins/builtins-utils.h.

https://codereview.chromium.org/2698813004/diff/110001/src/builtins/builtins-...
src/builtins/builtins-sharedarraybuffer.cc:641: // Uint8Clamped functions
On 2017/02/17 19:44:41, binji wrote:
> Oops, the Uint8Clamped versions should have been removed. Might as well do it
> now!

Would you mind if I removed this in a follow-on patch? This is such a big
refactoring; it'd be easier if I can leave the tests as is so that I can have
the confidence that I didn't mess up both the code and the tests.

[commit-bot: I haz the power, 2017-02-20 21:37:07]

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

[commit-bot: I haz the power, 2017-02-20 21:37:08]

Dry run: Try jobs failed on following builders:
  v8_android_arm_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_android_arm_compile_rel/...)
  v8_linux64_avx2_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng/buil...)
  v8_linux64_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...)
  v8_linux64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/21353)
  v8_linux64_verify_csa_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_verify_csa_rel_n...)
  v8_linux_arm_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm_rel_ng/builds/...)
  v8_linux_dbg_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng/builds/21375)
  v8_linux_gcc_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_gcc_compile_rel/bu...)
  v8_linux_mips64el_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mips64el_compile_r...)
  v8_linux_mipsel_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mipsel_compile_rel...)
  v8_linux_verify_csa_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_verify_csa_rel_ng/...)

[Dan Ehrenberg, 2017-02-20 21:38:56]

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

[commit-bot: I haz the power, 2017-02-20 21:38:57]

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-02-20 22:03:13]

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

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

Dry run: This issue passed the CQ dry run.

[Dan Ehrenberg, 2017-02-20 22:07:07]

The CQ bit was checked by littledan@chromium.org

[Dan Ehrenberg, 2017-02-20 22:07:08]

The patchset sent to the CQ was uploaded after l-g-t-m from binji@chromium.org
Link to the patchset: https://codereview.chromium.org/2698813004/#ps150001
(title: "Complete renaming")

[commit-bot: I haz the power, 2017-02-20 22:07:14]

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-02-20 22:08:53]

CQ is committing da patch.

Bot data: {"patchset_id": 150001, "attempt_start_ts": 1487628427693840,
"parent_rev": "2fe02ed40b045b5dd28e12a38b9cd20fee53f8f4", "commit_rev":
"2b9840d86f3b51f42918b6bf1a06ff8b62b2464d"}

[commit-bot: I haz the power, 2017-02-20 22:08:58]

Description was changed from

==========
[SAB] Move Atomics builtins to C++

This patch refactors the Atomics builtins so that they are implemented
as C++ builtins rather than experimental JS builtins. Previously, each
of these functions called out to a runtime function, so no significant
change in performance is anticipated. The goal of this patch is to
remove the last user of experimental JS builtins so that the mechanism
can be removed, for performance reasons. The patch includes a drive-by
fix of a check-fail. For the most part, the patch is just moving code
without modification from runtime-atomics.cc to
builtins-sharedarraybuffer.cc .

BUG=v8:5880
==========

to

==========
[SAB] Move Atomics builtins to C++

This patch refactors the Atomics builtins so that they are implemented
as C++ builtins rather than experimental JS builtins. Previously, each
of these functions called out to a runtime function, so no significant
change in performance is anticipated. The goal of this patch is to
remove the last user of experimental JS builtins so that the mechanism
can be removed, for performance reasons. The patch includes a drive-by
fix of a check-fail. For the most part, the patch is just moving code
without modification from runtime-atomics.cc to
builtins-sharedarraybuffer.cc .

BUG=v8:5880

Review-Url: https://codereview.chromium.org/2698813004
Cr-Commit-Position: refs/heads/master@{#43335}
Committed:
https://chromium.googlesource.com/v8/v8/+/2b9840d86f3b51f42918b6bf1a06ff8b62b...
==========

[commit-bot: I haz the power, 2017-02-20 22:08:59]

Committed patchset #9 (id:150001) as
https://chromium.googlesource.com/v8/v8/+/2b9840d86f3b51f42918b6bf1a06ff8b62b...

[Yang, 2017-02-21 07:22:28]

On 2017/02/20 22:08:59, commit-bot: I haz the power wrote:
> Committed patchset #9 (id:150001) as
>
https://chromium.googlesource.com/v8/v8/+/2b9840d86f3b51f42918b6bf1a06ff8b62b...

Nice! Thanks for doing this, Dan!

[Michael Hablich, 2017-02-27 18:54:47]

A revert of this CL (patchset #10 id:170001) has been created in
https://codereview.chromium.org/2716253003/ by hablich@chromium.org.

The reason for reverting is: Seems to the culprit in
BUG=chromium:695653.

[Michael Hablich, 2017-02-27 19:09:36]

On 2017/02/27 at 18:54:47, Michael Hablich wrote:
> A revert of this CL (patchset #10 id:170001) has been created in
https://codereview.chromium.org/2716253003/ by hablich@chromium.org.
> 
> The reason for reverting is: Seems to the culprit in
> BUG=chromium:695653.

The revert does not apply cleanly. Would be great if somebody could take care of
this :-).

[Dan Ehrenberg, 2017-02-27 19:14:13]

On 2017/02/27 19:09:36, Michael Hablich wrote:
> On 2017/02/27 at 18:54:47, Michael Hablich wrote:
> > A revert of this CL (patchset #10 id:170001) has been created in
> https://codereview.chromium.org/2716253003/ by mailto:hablich@chromium.org.
> > 
> > The reason for reverting is: Seems to the culprit in
> > BUG=chromium:695653.
> 
> The revert does not apply cleanly. Would be great if somebody could take care
of
> this :-).

I'll take care of this in a few hours if binji doesn't get to it first. Sorry
about the regression.

[binji, 2017-02-27 20:28:32]

On 2017/02/27 19:14:13, Dan Ehrenberg wrote:
> On 2017/02/27 19:09:36, Michael Hablich wrote:
> > On 2017/02/27 at 18:54:47, Michael Hablich wrote:
> > > A revert of this CL (patchset #10 id:170001) has been created in
> > https://codereview.chromium.org/2716253003/ by mailto:hablich@chromium.org.
> > > 
> > > The reason for reverting is: Seems to the culprit in
> > > BUG=chromium:695653.
> > 
> > The revert does not apply cleanly. Would be great if somebody could take
care
> of
> > this :-).
> 
> I'll take care of this in a few hours if binji doesn't get to it first. Sorry
> about the regression.

I'm working on a revert now.