[promises] port NewPromiseCapability to TF

[promises] port NewPromiseCapability to TF

- Adds CodeAssembler::ConstructJS() to simplify calling JS functions as
constructors, used by NewPromiseCapability()
- Defines PromiseCapability as a special JSObject subclass, with a
non-exensible Map, and read-only non-configurable DataDescriptors which
point to its in-object fields. This allows its fields to be used by JS
builtins until there is no longer any need.

Currently, the performance benefit comes from
https://codereview.chromium.org/2567033003/, but does not appear to
regress performance in any significant way.

BUG=v8:5343
TBR=ulan@chromium.org

Review-Url: https://codereview.chromium.org/2567333002
Cr-Commit-Position: refs/heads/master@{#42014}
Committed: https://chromium.googlesource.com/v8/v8/+/4f95a1eb5fc22bef9a69e7856b03dbddb40e20ec

[caitp, 2016-12-13 09:11:03]

The CQ bit was checked by caitp@igalia.com to run a CQ dry run

[commit-bot: I haz the power, 2016-12-13 09:11: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, 2016-12-13 09:16:41]

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

[commit-bot: I haz the power, 2016-12-13 09:16:42]

Dry run: Try jobs failed on following builders:
  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...)

[caitp, 2016-12-13 16:05:34]

The CQ bit was checked by caitp@igalia.com to run a CQ dry run

[commit-bot: I haz the power, 2016-12-13 16:05:42]

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, 2016-12-13 16:11:13]

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

[commit-bot: I haz the power, 2016-12-13 16:11:14]

Dry run: Try jobs failed on following builders:
  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...)

[caitp, 2016-12-13 16:35:46]

The CQ bit was checked by caitp@igalia.com to run a CQ dry run

[commit-bot: I haz the power, 2016-12-13 16:35: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, 2016-12-13 16:41:25]

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

[commit-bot: I haz the power, 2016-12-13 16:41:26]

Dry run: Try jobs failed on following builders:
  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...)

[caitp, 2016-12-13 16:45:32]

The CQ bit was checked by caitp@igalia.com to run a CQ dry run

[commit-bot: I haz the power, 2016-12-13 16:45:36]

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, 2016-12-13 17:12:50]

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

[commit-bot: I haz the power, 2016-12-13 17:12:50]

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

[caitp, 2016-12-13 17:47:42]

The CQ bit was checked by caitp@igalia.com to run a CQ dry run

[commit-bot: I haz the power, 2016-12-13 17:47:46]

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, 2016-12-13 18:19:54]

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

[commit-bot: I haz the power, 2016-12-13 18:19:55]

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

[caitp, 2016-12-14 17:02:43]

caitp@igalia.com changed reviewers:
+ bmeurer@chromium.org, gsathya@chromium.org

[caitp, 2016-12-14 17:02:44]

Hi, PTAL

I'm wondering if anyone knows if CSA CallRuntime actually works with the arm64
simulator or not, because it seems to break every time here, and I'm not sure
why.

[caitp, 2016-12-14 19:12:58]

Description was changed from

==========
[promises] port NewPromiseCapability to TF

- Adds CodeAssembler::ConstructJS() to simplify calling JS functions as
constructors, used by NewPromiseCapability()
- Defines PromiseCapability as a special JSObject subclass, with a
non-exensible Map, and read-only non-configurable DataDescriptors which
point to its in-object fields. This allows its fields to be used by JS
builtins until there is no longer any need.

Currently, the performance benefit comes from
https://codereview.chromium.org/2567033003/, but does not appear to
regress performance in any significant way.

BUG=v8:5343
==========

to

==========
[promises] port NewPromiseCapability to TF

- Adds CodeAssembler::ConstructJS() to simplify calling JS functions as
constructors, used by NewPromiseCapability()
- Defines PromiseCapability as a special JSObject subclass, with a
non-exensible Map, and read-only non-configurable DataDescriptors which
point to its in-object fields. This allows its fields to be used by JS
builtins until there is no longer any need.

Currently, the performance benefit comes from
https://codereview.chromium.org/2567033003/, but does not appear to
regress performance in any significant way.

BUG=v8:5343
==========

[caitp, 2016-12-14 19:12:58]

caitp@igalia.com changed reviewers:
+ ishell@chromium.org

[caitp, 2016-12-15 21:36:08]

The CQ bit was checked by caitp@igalia.com to run a CQ dry run

[commit-bot: I haz the power, 2016-12-15 21:36:13]

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, 2016-12-15 21:42:14]

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

[commit-bot: I haz the power, 2016-12-15 21:42:16]

Dry run: Try jobs failed on following builders:
  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...)

[caitp, 2016-12-15 21:42:44]

The CQ bit was checked by caitp@igalia.com to run a CQ dry run

[commit-bot: I haz the power, 2016-12-15 21:42:55]

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, 2016-12-15 22:19:40]

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

[commit-bot: I haz the power, 2016-12-15 22:19:41]

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

[caitp, 2016-12-16 20:52:35]

The CQ bit was checked by caitp@igalia.com to run a CQ dry run

[commit-bot: I haz the power, 2016-12-16 20:52:46]

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, 2016-12-16 20:54:15]

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

[commit-bot: I haz the power, 2016-12-16 20:54:16]

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_linux_arm64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel_ng/build...)
  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_nodcheck_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_nodcheck_rel_ng/bu...)
  v8_linux_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_rel_ng/builds/17964)
  v8_presubmit on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/30860)
  v8_win64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win64_rel_ng/builds/19624)
  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_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win_rel_ng/builds/19701)

[caitp, 2016-12-16 20:57:34]

The CQ bit was checked by caitp@igalia.com to run a CQ dry run

[commit-bot: I haz the power, 2016-12-16 20:57: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, 2016-12-16 21:17:49]

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

[commit-bot: I haz the power, 2016-12-16 21:17:50]

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/19625)
  v8_win64_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win64_rel_ng_triggered/b...)

[caitp, 2016-12-16 22:15:57]

The CQ bit was checked by caitp@igalia.com to run a CQ dry run

[commit-bot: I haz the power, 2016-12-16 22:16:04]

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, 2016-12-16 22:40:20]

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

[commit-bot: I haz the power, 2016-12-16 22:40:21]

Dry run: Try jobs failed on following builders:
  v8_linux_dbg_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng/builds/18062)
  v8_linux_dbg_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng_triggered/b...)

[caitp, 2016-12-16 23:07:32]

The CQ bit was checked by caitp@igalia.com to run a CQ dry run

[commit-bot: I haz the power, 2016-12-16 23:07:37]

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, 2016-12-17 00:02:00]

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

[commit-bot: I haz the power, 2016-12-17 00:02:01]

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

[caitp, 2016-12-17 00:18:20]

On 2016/12/14 17:02:44, caitp wrote:
> Hi, PTAL
> 
> I'm wondering if anyone knows if CSA CallRuntime actually works with the arm64
> simulator or not, because it seems to break every time here, and I'm not sure
> why.

I'm thinking this is related to the kRestoreJSSP / kRestoreCSP stuff, and most
likely a CallRuntime in the new builtins is clobbering one of those registers,
but I'm not exactly sure what the problem is, since there's nothing in the new
code that's really different from existing builtin code.

[Benedikt Meurer, 2016-12-17 11:45:06]

bmeurer@chromium.org changed reviewers:
+ rodolph.perfetta@arm.com

[Benedikt Meurer, 2016-12-17 11:45:07]

Rodolph: Can you shed some light on this?

[Rodolph Perfetta, 2016-12-19 13:47:10]

On 2016/12/17 11:45:07, Benedikt Meurer wrote:
> Rodolph: Can you shed some light on this?

We are looking into this.

Rodolph.

[caitp, 2016-12-19 23:38:53]

On 2016/12/19 13:47:10, Rodolph Perfetta wrote:
> On 2016/12/17 11:45:07, Benedikt Meurer wrote:
> > Rodolph: Can you shed some light on this?
> 
> We are looking into this.
> 
> Rodolph.

Looks like the crash comes after the ConstructJS call in NewPromiseCapability.

I _think_ jssp is corrupted, because below, x2 no longer points to the allocated
JSPromiseCapability object. So, maybe ConstructJS needs some changes to
accommodate ARM, but I'm not sure what they'd be. 

```
// ConstructJS()
0x106221174   692  5800d9b0       ldr x16, pc+6964 (addr 0x0000000106222ca8)   
;; code: BUILTIN
0x106221178   696  d63f0200       blr x16
0x10622117c   700  f9402382       ldr x2, [jssp, #64]

// CRASH
0x106221180   704  f841f041       ldur x1, [x2, #31]
```

[georgia.kouveli, 2016-12-20 15:38:16]

On 2016/12/19 23:38:53, caitp wrote:
> On 2016/12/19 13:47:10, Rodolph Perfetta wrote:
> > On 2016/12/17 11:45:07, Benedikt Meurer wrote:
> > > Rodolph: Can you shed some light on this?
> > 
> > We are looking into this.
> > 
> > Rodolph.
> 
> Looks like the crash comes after the ConstructJS call in NewPromiseCapability.
> 
> I _think_ jssp is corrupted, because below, x2 no longer points to the
allocated
> JSPromiseCapability object. So, maybe ConstructJS needs some changes to
> accommodate ARM, but I'm not sure what they'd be. 
> 
> ```
> // ConstructJS()
> 0x106221174   692  5800d9b0       ldr x16, pc+6964 (addr 0x0000000106222ca8)  

> ;; code: BUILTIN
> 0x106221178   696  d63f0200       blr x16
> 0x10622117c   700  f9402382       ldr x2, [jssp, #64]
> 
> // CRASH
> 0x106221180   704  f841f041       ldur x1, [x2, #31]
> ```

NewPromiseCapability seems to be pushing one argument fewer on the stack when
calling Construct than what is popped by Construct and the functions/stubs it
calls. The argument appears to be the receiver (which I suppose must be pushed
on the stack at some point, since Construct works correctly, but I am not sure
when). Construct jumps into ConstructFunction, which jumps into
JSBuiltinsConstructStubForDerived. At the end of that stub, the arguments are
popped from the stack (in this case only one argument, the "executor"), together
with the receiver (see the Drop(1) in Generate_JSConstructStubHelper in
src/builtins/arm64/builtins-arm64.cc).This means that JSSP is not restored to
the same value it had before the ConstructJS call. This happens on arm as well
with SP, and as far as I can tell, on x64. (I guessed that the popped argument
is the receiver based on a comment in the ia32 code.)

The difference with arm64 is that the arm64 backend performs an optimization
where it uses JSSP instead of FP if it's beneficial (see SlotToMemOperand() in
src/compiler/arm64/code-generator-arm64.cc). If you disable this optimisation
(which doesn't seem to happen for x64 and arm), the test works. However the real
issue is that frame_access_state()->GetSPToFPOffset() is returning the wrong
number (it returns 96 when the offset between the fp and sp is now 88 due to
this mismatch in pushing/popping arguments). This appears to happen on arm too
in this particular case, but does not cause an issue since we just use the fp to
access the stack. Perhaps something is wrong with the way Construct is called?

[caitp, 2016-12-20 15:55:47]

On 2016/12/20 15:38:16, georgia.kouveli wrote:
> On 2016/12/19 23:38:53, caitp wrote:
> > On 2016/12/19 13:47:10, Rodolph Perfetta wrote:
> > > On 2016/12/17 11:45:07, Benedikt Meurer wrote:
> > > > Rodolph: Can you shed some light on this?
> > > 
> > > We are looking into this.
> > > 
> > > Rodolph.
> > 
> > Looks like the crash comes after the ConstructJS call in
NewPromiseCapability.
> > 
> > I _think_ jssp is corrupted, because below, x2 no longer points to the
> allocated
> > JSPromiseCapability object. So, maybe ConstructJS needs some changes to
> > accommodate ARM, but I'm not sure what they'd be. 
> > 
> > ```
> > // ConstructJS()
> > 0x106221174   692  5800d9b0       ldr x16, pc+6964 (addr 0x0000000106222ca8)
 
> 
> > ;; code: BUILTIN
> > 0x106221178   696  d63f0200       blr x16
> > 0x10622117c   700  f9402382       ldr x2, [jssp, #64]
> > 
> > // CRASH
> > 0x106221180   704  f841f041       ldur x1, [x2, #31]
> > ```
> 
> NewPromiseCapability seems to be pushing one argument fewer on the stack when
> calling Construct than what is popped by Construct and the functions/stubs it
> calls. The argument appears to be the receiver (which I suppose must be pushed
> on the stack at some point, since Construct works correctly, but I am not sure
> when). Construct jumps into ConstructFunction, which jumps into
> JSBuiltinsConstructStubForDerived. At the end of that stub, the arguments are
> popped from the stack (in this case only one argument, the "executor"),
together
> with the receiver (see the Drop(1) in Generate_JSConstructStubHelper in
> src/builtins/arm64/builtins-arm64.cc).This means that JSSP is not restored to
> the same value it had before the ConstructJS call. This happens on arm as well
> with SP, and as far as I can tell, on x64. (I guessed that the popped argument
> is the receiver based on a comment in the ia32 code.)
> 
> The difference with arm64 is that the arm64 backend performs an optimization
> where it uses JSSP instead of FP if it's beneficial (see SlotToMemOperand() in
> src/compiler/arm64/code-generator-arm64.cc). If you disable this optimisation
> (which doesn't seem to happen for x64 and arm), the test works. However the
real
> issue is that frame_access_state()->GetSPToFPOffset() is returning the wrong
> number (it returns 96 when the offset between the fp and sp is now 88 due to
> this mismatch in pushing/popping arguments). This appears to happen on arm too
> in this particular case, but does not cause an issue since we just use the fp
to
> access the stack. Perhaps something is wrong with the way Construct is called?

Makes sense, thanks for looking into that.

[caitp, 2016-12-20 16:53:35]

The CQ bit was checked by caitp@igalia.com to run a CQ dry run

[commit-bot: I haz the power, 2016-12-20 16:53:46]

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, 2016-12-20 17:11:26]

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

[commit-bot: I haz the power, 2016-12-20 17:11:27]

Dry run: Try jobs failed on following builders:
  v8_linux64_asan_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel_ng/buil...)
  v8_linux64_asan_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel_ng_trig...)

[caitp, 2016-12-20 20:01:57]

The CQ bit was checked by caitp@igalia.com to run a CQ dry run

[commit-bot: I haz the power, 2016-12-20 20:02:04]

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, 2016-12-20 20:37:24]

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

[commit-bot: I haz the power, 2016-12-20 20:37:25]

Dry run: This issue passed the CQ dry run.

[gsathya, 2016-12-21 01:02:33]

Nice! once https://codereview.chromium.org/2590563003/ lands we can remove
NewInternalPromiseCapability

https://codereview.chromium.org/2567333002/diff/300001/src/bootstrapper.cc
File src/bootstrapper.cc (right):

https://codereview.chromium.org/2567333002/diff/300001/src/bootstrapper.cc#ne...
src/bootstrapper.cc:1849: isolate);
Handle<Code> code = isolate->builtins()->PromiseGetCapabilitiesExecutor();

https://codereview.chromium.org/2567333002/diff/300001/src/builtins/builtins-...
File src/builtins/builtins-promise.cc (right):

https://codereview.chromium.org/2567333002/diff/300001/src/builtins/builtins-...
src/builtins/builtins-promise.cc:33: StoreContextElement(context,
Context::NATIVE_CONTEXT_INDEX, native_context);
You refactor this to reuse CreatePromiseResolvingFunctionsContext (like a common
CreatePromiseContext)

https://codereview.chromium.org/2567333002/diff/300001/src/builtins/builtins-...
src/builtins/builtins-promise.cc:34: StoreContextElement(context,
GetPromiseCapabilityExecutor::kCapabilitySlot,
StoreContextElementNoWriteBarrier

https://codereview.chromium.org/2567333002/diff/300001/src/builtins/builtins-...
src/builtins/builtins-promise.cc:49: StoreMapNoWriteBarrier(capability,
Heap::kJSPromiseCapabilityMapRootIndex);
Why not use AllocateJSObjectFromMap here?

https://codereview.chromium.org/2567333002/diff/300001/src/code-stub-assemble...
File src/code-stub-assembler.cc (right):

https://codereview.chromium.org/2567333002/diff/300001/src/code-stub-assemble...
src/code-stub-assembler.cc:8: #include "src/promise-utils.h"
Is this needed?

https://codereview.chromium.org/2567333002/diff/300001/src/heap/heap.cc
File src/heap/heap.cc (right):

https://codereview.chromium.org/2567333002/diff/300001/src/heap/heap.cc#newco...
src/heap/heap.cc:2910: }
Honestly I feel like we could just use runtime calls for now

[Igor Sheludko, 2016-12-21 12:00:45]

https://codereview.chromium.org/2567333002/diff/300001/src/builtins/builtins-...
File src/builtins/builtins-promise.cc (right):

https://codereview.chromium.org/2567333002/diff/300001/src/builtins/builtins-...
src/builtins/builtins-promise.cc:34: StoreContextElement(context,
GetPromiseCapabilityExecutor::kCapabilitySlot,
On 2016/12/21 01:02:33, gsathya wrote:
> StoreContextElementNoWriteBarrier

We can use it for all the stores because the object is small enough to be
allocated in new space.

https://codereview.chromium.org/2567333002/diff/300001/src/code-stub-assembler.h
File src/code-stub-assembler.h (right):

https://codereview.chromium.org/2567333002/diff/300001/src/code-stub-assemble...
src/code-stub-assembler.h:1212: int argc_index = state->parameter_count() - 2;  
          \
Please add a comment:
  // See Linkage::GetJSCallDescriptor().

[gsathya, 2016-12-21 19:43:25]

https://codereview.chromium.org/2567333002/diff/300001/src/builtins/builtins.h
File src/builtins/builtins.h (right):

https://codereview.chromium.org/2567333002/diff/300001/src/builtins/builtins....
src/builtins/builtins.h:575: TFJ(CreateResolvingFunctions, 2)                   
                         \
Can we remove this builtin?

[caitp, 2016-12-21 20:59:37]

The CQ bit was checked by caitp@igalia.com to run a CQ dry run

[commit-bot: I haz the power, 2016-12-21 20:59:44]

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

[caitp, 2016-12-21 21:01:13]

think those are taken care of

https://codereview.chromium.org/2567333002/diff/300001/src/bootstrapper.cc
File src/bootstrapper.cc (right):

https://codereview.chromium.org/2567333002/diff/300001/src/bootstrapper.cc#ne...
src/bootstrapper.cc:1849: isolate);
On 2016/12/21 01:02:33, gsathya wrote:
> Handle<Code> code = isolate->builtins()->PromiseGetCapabilitiesExecutor();

Done.

https://codereview.chromium.org/2567333002/diff/300001/src/builtins/builtins-...
File src/builtins/builtins-promise.cc (right):

https://codereview.chromium.org/2567333002/diff/300001/src/builtins/builtins-...
src/builtins/builtins-promise.cc:49: StoreMapNoWriteBarrier(capability,
Heap::kJSPromiseCapabilityMapRootIndex);
On 2016/12/21 01:02:33, gsathya wrote:
> Why not use AllocateJSObjectFromMap here?

Done.

https://codereview.chromium.org/2567333002/diff/300001/src/builtins/builtins.h
File src/builtins/builtins.h (right):

https://codereview.chromium.org/2567333002/diff/300001/src/builtins/builtins....
src/builtins/builtins.h:575: TFJ(CreateResolvingFunctions, 2)                   
                         \
On 2016/12/21 19:43:25, gsathya wrote:
> Can we remove this builtin?

Done.

https://codereview.chromium.org/2567333002/diff/300001/src/heap/heap.cc
File src/heap/heap.cc (right):

https://codereview.chromium.org/2567333002/diff/300001/src/heap/heap.cc#newco...
src/heap/heap.cc:2910: }
On 2016/12/21 01:02:33, gsathya wrote:
> Honestly I feel like we could just use runtime calls for now

To me, it seemed simpler to let it use in-object stuff. Once there's no JS code
that needs to look at these objects, it will be safe to get rid of this gunk and
make the PromiseCapability a Struct with a dead simple Map, and slightly smaller
size on the heap.

[Igor Sheludko, 2016-12-27 14:14:02]

lgtm with a nit:

https://codereview.chromium.org/2567333002/diff/340001/src/code-stub-assembler.h
File src/code-stub-assembler.h (right):

https://codereview.chromium.org/2567333002/diff/340001/src/code-stub-assemble...
src/code-stub-assembler.h:1212: int argc_index = state->parameter_count() - 2;  
          \
Please add a comment:
  // See Linkage::GetJSCallDescriptor().

[gsathya, 2016-12-29 21:48:47]

lgtm

https://codereview.chromium.org/2567333002/diff/340001/src/builtins/builtins-...
File src/builtins/builtins-promise.cc (right):

https://codereview.chromium.org/2567333002/diff/340001/src/builtins/builtins-...
src/builtins/builtins-promise.cc:41: Label if_builtin_promise(this),
if_custom_promise(this), out(this);
if_custom_promise can be deferred

https://codereview.chromium.org/2567333002/diff/340001/src/objects-debug.cc
File src/objects-debug.cc (right):

https://codereview.chromium.org/2567333002/diff/340001/src/objects-debug.cc#n...
src/objects-debug.cc:891: Isolate* const isolate = GetIsolate();
do you need the isolate here?

https://codereview.chromium.org/2567333002/diff/340001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/2567333002/diff/340001/src/objects.h#newcode8627
src/objects.h:8627: class JSPromiseCapability : public JSObject {
can you please add a TODO to make this into a struct?

[gsathya, 2016-12-30 02:35:42]

https://codereview.chromium.org/2567333002/diff/340001/src/builtins/builtins-...
File src/builtins/builtins-promise.h (right):

https://codereview.chromium.org/2567333002/diff/340001/src/builtins/builtins-...
src/builtins/builtins-promise.h:44: Node* CreatePromiseContext(Node*
native_context, int slots);
Can you please add tests for this and the others?

[Benedikt Meurer, 2016-12-30 07:39:59]

LGTM on compiler.

[caitp, 2016-12-30 16:11:01]

On 2016/12/30 07:39:59, Benedikt Meurer wrote:
> LGTM on compiler.

I'll finish this up on Monday if that works for people. If there's a pressing
need for landing this I'll make some time for it today

[caitp, 2017-01-02 13:22:49]

The CQ bit was checked by caitp@igalia.com to run a CQ dry run

[commit-bot: I haz the power, 2017-01-02 13:22:55]

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-01-02 13:50:33]

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

[commit-bot: I haz the power, 2017-01-02 13:50:34]

Dry run: This issue passed the CQ dry run.

[caitp, 2017-01-02 15:48:56]

The CQ bit was checked by caitp@igalia.com to run a CQ dry run

[commit-bot: I haz the power, 2017-01-02 15:49:05]

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

[caitp, 2017-01-02 15:51:12]

I think this is ready to go, will CQ shortly.

https://codereview.chromium.org/2567333002/diff/340001/src/builtins/builtins-...
File src/builtins/builtins-promise.h (right):

https://codereview.chromium.org/2567333002/diff/340001/src/builtins/builtins-...
src/builtins/builtins-promise.h:44: Node* CreatePromiseContext(Node*
native_context, int slots);
On 2016/12/30 02:35:42, gsathya wrote:
> Can you please add tests for this and the others?

Added tests for NewPromiseCapability and
CreatePromiseGetCapabilitiesExecutorContext (NewInternalPromiseCapability is
removed in latest patchset, and CreatePromiseContext() is just a helper for the
common code between the 2 promise context types, so it has coverage already).

https://codereview.chromium.org/2567333002/diff/340001/src/code-stub-assembler.h
File src/code-stub-assembler.h (right):

https://codereview.chromium.org/2567333002/diff/340001/src/code-stub-assemble...
src/code-stub-assembler.h:1212: int argc_index = state->parameter_count() - 2;  
          \
On 2016/12/27 14:14:02, Igor Sheludko wrote:
> Please add a comment:
>   // See Linkage::GetJSCallDescriptor().

Done.

https://codereview.chromium.org/2567333002/diff/340001/src/objects-debug.cc
File src/objects-debug.cc (right):

https://codereview.chromium.org/2567333002/diff/340001/src/objects-debug.cc#n...
src/objects-debug.cc:891: Isolate* const isolate = GetIsolate();
On 2016/12/29 21:48:47, gsathya wrote:
> do you need the isolate here?

Oop, done

https://codereview.chromium.org/2567333002/diff/340001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/2567333002/diff/340001/src/objects.h#newcode8627
src/objects.h:8627: class JSPromiseCapability : public JSObject {
On 2016/12/29 21:48:47, gsathya wrote:
> can you please add a TODO to make this into a struct?

Done.

[commit-bot: I haz the power, 2017-01-02 15:53:55]

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

[commit-bot: I haz the power, 2017-01-02 15:53:56]

Dry run: Try jobs failed on following builders:
  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...)

[caitp, 2017-01-02 15:56:24]

The CQ bit was checked by caitp@igalia.com to run a CQ dry run

[commit-bot: I haz the power, 2017-01-02 15:56: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-01-02 16:33:57]

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

[commit-bot: I haz the power, 2017-01-02 16:33:58]

Dry run: Try jobs failed on following builders:
  v8_linux_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_rel_ng/builds/18449)
  v8_linux_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_rel_ng_triggered/b...)

[caitp, 2017-01-02 16:43:49]

The CQ bit was checked by caitp@igalia.com to run a CQ dry run

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

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

[commit-bot: I haz the power, 2017-01-02 17:08:24]

Dry run: This issue passed the CQ dry run.

[caitp, 2017-01-02 17:16:08]

The CQ bit was checked by caitp@igalia.com

[caitp, 2017-01-02 17:16:09]

The patchset sent to the CQ was uploaded after l-g-t-m from
bmeurer@chromium.org, ishell@chromium.org, gsathya@chromium.org
Link to the patchset: https://codereview.chromium.org/2567333002/#ps420001
(title: "Make gcmole happy")

[commit-bot: I haz the power, 2017-01-02 17:16:13]

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

[Rodolph.Perfetta_arm.com, 2017-01-02 17:16:22]

I am out of office until the 2nd of January 2017  I will reply to your message
on my return.

Regards,
Rodolph.
IMPORTANT NOTICE: The contents of this email and any attachments are
confidential and may also be privileged. If you are not the intended recipient,
please notify the sender immediately and do not disclose the contents to any
other person, use it for any purpose, or store or copy the information in any
medium. Thank you.

[commit-bot: I haz the power, 2017-01-02 17:19:22]

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

[commit-bot: I haz the power, 2017-01-02 17:19:23]

Try jobs failed on following builders:
  v8_presubmit on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/31498)

[caitp, 2017-01-02 17:20:37]

Description was changed from

==========
[promises] port NewPromiseCapability to TF

- Adds CodeAssembler::ConstructJS() to simplify calling JS functions as
constructors, used by NewPromiseCapability()
- Defines PromiseCapability as a special JSObject subclass, with a
non-exensible Map, and read-only non-configurable DataDescriptors which
point to its in-object fields. This allows its fields to be used by JS
builtins until there is no longer any need.

Currently, the performance benefit comes from
https://codereview.chromium.org/2567033003/, but does not appear to
regress performance in any significant way.

BUG=v8:5343
==========

to

==========
[promises] port NewPromiseCapability to TF

- Adds CodeAssembler::ConstructJS() to simplify calling JS functions as
constructors, used by NewPromiseCapability()
- Defines PromiseCapability as a special JSObject subclass, with a
non-exensible Map, and read-only non-configurable DataDescriptors which
point to its in-object fields. This allows its fields to be used by JS
builtins until there is no longer any need.

Currently, the performance benefit comes from
https://codereview.chromium.org/2567033003/, but does not appear to
regress performance in any significant way.

BUG=v8:5343
TBR=ulan@chromium.org
==========

[caitp, 2017-01-02 17:20:47]

The CQ bit was checked by caitp@igalia.com

[commit-bot: I haz the power, 2017-01-02 17:20:53]

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-01-02 17:22:31]

CQ is committing da patch.

Bot data: {"patchset_id": 420001, "attempt_start_ts": 1483377647074050,
"parent_rev": "587fda09b72b1b73573860be41978bd5ebdfff2c", "commit_rev":
"4f95a1eb5fc22bef9a69e7856b03dbddb40e20ec"}

[commit-bot: I haz the power, 2017-01-02 17:22:36]

Description was changed from

==========
[promises] port NewPromiseCapability to TF

- Adds CodeAssembler::ConstructJS() to simplify calling JS functions as
constructors, used by NewPromiseCapability()
- Defines PromiseCapability as a special JSObject subclass, with a
non-exensible Map, and read-only non-configurable DataDescriptors which
point to its in-object fields. This allows its fields to be used by JS
builtins until there is no longer any need.

Currently, the performance benefit comes from
https://codereview.chromium.org/2567033003/, but does not appear to
regress performance in any significant way.

BUG=v8:5343
TBR=ulan@chromium.org
==========

to

==========
[promises] port NewPromiseCapability to TF

- Adds CodeAssembler::ConstructJS() to simplify calling JS functions as
constructors, used by NewPromiseCapability()
- Defines PromiseCapability as a special JSObject subclass, with a
non-exensible Map, and read-only non-configurable DataDescriptors which
point to its in-object fields. This allows its fields to be used by JS
builtins until there is no longer any need.

Currently, the performance benefit comes from
https://codereview.chromium.org/2567033003/, but does not appear to
regress performance in any significant way.

BUG=v8:5343
TBR=ulan@chromium.org

Review-Url: https://codereview.chromium.org/2567333002
Cr-Commit-Position: refs/heads/master@{#42014}
Committed:
https://chromium.googlesource.com/v8/v8/+/4f95a1eb5fc22bef9a69e7856b03dbddb40...
==========

[commit-bot: I haz the power, 2017-01-02 17:22:38]

Committed patchset #22 (id:420001) as
https://chromium.googlesource.com/v8/v8/+/4f95a1eb5fc22bef9a69e7856b03dbddb40...