Allow embedder to set promise internal field count

Allow embedder to set promise internal field count

Asynchronous context tracking mechanisms in Node.js need to store some
state on all promise objects. This change will allow embedders to
configure the number of internal fields on promises as is already done
for ArrayBuffers.

BUG=v8:6435
Review-Url: https://codereview.chromium.org/2889863002
Cr-Commit-Position: refs/heads/master@{#45496}
Committed: https://chromium.googlesource.com/v8/v8/+/6803eef1424979ff787aaf4e3b604594f535c384

[mattloring, 2017-05-18 16:35:48]

Description was changed from

==========
Allow embedder to set promise internal field count

Asynchronous context tracking mechanisms in Node.js need to store some
state on all promise objects. This change will allow embedders to
configure the number of internal fields on promises as is already done
for ArrayBuffers.

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

to

==========
Allow embedder to set promise internal field count

Asynchronous context tracking mechanisms in Node.js need to store some
state on all promise objects. This change will allow embedders to
configure the number of internal fields on promises as is already done
for ArrayBuffers.

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

[mattloring, 2017-05-18 16:35:49]

mattloring@google.com changed reviewers:
+ gsathya@chromium.org, jochen@chromium.org

[mattloring, 2017-05-18 16:36:39]

mattloring@google.com changed reviewers:
+ ofrobots@chromium.org

[gsathya, 2017-05-18 16:48:33]

I'm not sure of how the embedder fields work (-- if there are any other changes
required for the GC). Will let jochen@ review 

You need to change Factory::NewJSPromise as well

https://codereview.chromium.org/2889863002/diff/1/src/builtins/builtins-promi...
File src/builtins/builtins-promise-gen.cc (right):

https://codereview.chromium.org/2889863002/diff/1/src/builtins/builtins-promi...
src/builtins/builtins-promise-gen.cc:20: // Does this need to change to account
for new internal fields?
Nope this is fine, just the init code below.

[Benedikt Meurer, 2017-05-18 17:22:46]

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

[jochen (gone - plz use gerrit), 2017-05-18 20:38:33]

yeah, that looks about right

Sathya, Factory::NewJSPromise just calls the constructor function, so it should
also invoke the init that clears the internal fields, no?

[gsathya, 2017-05-19 03:20:27]

On 2017/05/18 20:38:33, jochen wrote:
> yeah, that looks about right
> 
> Sathya, Factory::NewJSPromise just calls the constructor function, so it
should
> also invoke the init that clears the internal fields, no?

No only the api calls the constructor  --
https://cs.chromium.org/chromium/src/v8/src/api.cc?l=7414

Factory doesn't -- https://cs.chromium.org/chromium/src/v8/src/factory.cc?l=919

[mattloring, 2017-05-19 17:57:58]

On 2017/05/19 03:20:27, gsathya wrote:
> On 2017/05/18 20:38:33, jochen wrote:
> > yeah, that looks about right
> > 
> > Sathya, Factory::NewJSPromise just calls the constructor function, so it
> should
> > also invoke the init that clears the internal fields, no?
> 
> No only the api calls the constructor  --
> https://cs.chromium.org/chromium/src/v8/src/api.cc?l=7414
> 
> Factory doesn't --
https://cs.chromium.org/chromium/src/v8/src/factory.cc?l=919

What is the correct way to set internal fields from factory.cc (the analogue of
StoreObjectFieldNoWriteBarrier)? Most of the initialization seems to be done
with named setters.

[jochen (gone - plz use gerrit), 2017-05-22 11:57:04]

for array buffers we invoke this from factory:
https://cs.chromium.org/chromium/src/v8/src/objects.cc?rcl=fbbc12b378aee6c0bd...

[mattloring, 2017-05-22 17:43:30]

On 2017/05/22 11:57:04, jochen wrote:
> for array buffers we invoke this from factory:
>
https://cs.chromium.org/chromium/src/v8/src/objects.cc?rcl=fbbc12b378aee6c0bd...

Thanks, I've updated the initialization in factory. Do you have any thoughts on
how this should be tested? I can't find any tests that set
V8_ARRAY_BUFFER_INTERNAL_FIELD_COUNT.

[mattloring, 2017-05-22 18:32:49]

On 2017/05/22 17:43:30, mattloring wrote:
> On 2017/05/22 11:57:04, jochen wrote:
> > for array buffers we invoke this from factory:
> >
>
https://cs.chromium.org/chromium/src/v8/src/objects.cc?rcl=fbbc12b378aee6c0bd...
> 
> Thanks, I've updated the initialization in factory. Do you have any thoughts
on
> how this should be tested? I can't find any tests that set
> V8_ARRAY_BUFFER_INTERNAL_FIELD_COUNT.

After talking with Jochen, it sounds like this is something better tested on the
Node.js side.

[gsathya, 2017-05-22 18:38:33]

lgtm

please wait for lgtm from jochen as well

https://codereview.chromium.org/2889863002/diff/20001/src/builtins/builtins-p...
File src/builtins/builtins-promise-gen.cc (right):

https://codereview.chromium.org/2889863002/diff/20001/src/builtins/builtins-p...
src/builtins/builtins-promise-gen.cc:59: Node*
PromiseBuiltinsAssembler::AllocateAndSetJSPromise(Node* context,
This needs to be updated as well

[mattloring, 2017-05-22 19:10:49]

On 2017/05/22 18:38:33, gsathya wrote:
> lgtm
> 
> please wait for lgtm from jochen as well
> 
>
https://codereview.chromium.org/2889863002/diff/20001/src/builtins/builtins-p...
> File src/builtins/builtins-promise-gen.cc (right):
> 
>
https://codereview.chromium.org/2889863002/diff/20001/src/builtins/builtins-p...
> src/builtins/builtins-promise-gen.cc:59: Node*
> PromiseBuiltinsAssembler::AllocateAndSetJSPromise(Node* context,
> This needs to be updated as well

Done.

[jochen (gone - plz use gerrit), 2017-05-23 11:19:50]

lgtm with build.gn changes done

https://codereview.chromium.org/2889863002/diff/70001/BUILD.gn
File BUILD.gn (right):

https://codereview.chromium.org/2889863002/diff/70001/BUILD.gn#newcode49
BUILD.gn:49: v8_promise_internal_field_count = ""
that should be a number, no?

https://codereview.chromium.org/2889863002/diff/70001/BUILD.gn#newcode213
BUILD.gn:213: defines += [ "V8_PROMISE_INTERNAL_FIELD_COUNT" ]
and that should be
V8_PROMISE_INTERNAL_FIELD_COUNT=${v8_promise_internal_field_count}

[mattloring, 2017-05-23 16:29:08]

Description was changed from

==========
Allow embedder to set promise internal field count

Asynchronous context tracking mechanisms in Node.js need to store some
state on all promise objects. This change will allow embedders to
configure the number of internal fields on promises as is already done
for ArrayBuffers.

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

to

==========
Allow embedder to set promise internal field count

Asynchronous context tracking mechanisms in Node.js need to store some
state on all promise objects. This change will allow embedders to
configure the number of internal fields on promises as is already done
for ArrayBuffers.

BUG=v8:6435
==========

[mattloring, 2017-05-23 16:54:43]

https://codereview.chromium.org/2889863002/diff/70001/BUILD.gn
File BUILD.gn (right):

https://codereview.chromium.org/2889863002/diff/70001/BUILD.gn#newcode49
BUILD.gn:49: v8_promise_internal_field_count = ""
On 2017/05/23 11:19:49, jochen wrote:
> that should be a number, no?

Done.

https://codereview.chromium.org/2889863002/diff/70001/BUILD.gn#newcode213
BUILD.gn:213: defines += [ "V8_PROMISE_INTERNAL_FIELD_COUNT" ]
On 2017/05/23 11:19:49, jochen wrote:
> and that should be
> V8_PROMISE_INTERNAL_FIELD_COUNT=${v8_promise_internal_field_count}

Done.

[mattloring, 2017-05-23 16:54:50]

The CQ bit was checked by mattloring@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-05-23 16:55:01]

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-05-23 16:56:29]

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

[commit-bot: I haz the power, 2017-05-23 16:56:30]

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_gcc_compile_dbg on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gcc_compile_dbg/...)
  v8_linux64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/26426)
  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_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_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/26426)
  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_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/26332)
  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/...)
  v8_mac_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/22985)

[mattloring, 2017-05-23 16:59:10]

The CQ bit was checked by mattloring@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-05-23 16:59:20]

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-05-23 17:33:17]

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

[commit-bot: I haz the power, 2017-05-23 17:33:18]

Dry run: This issue passed the CQ dry run.

[mattloring, 2017-05-23 17:52:39]

The CQ bit was checked by mattloring@google.com

[mattloring, 2017-05-23 17:52:40]

The patchset sent to the CQ was uploaded after l-g-t-m from
gsathya@chromium.org, jochen@chromium.org
Link to the patchset: https://codereview.chromium.org/2889863002/#ps110001
(title: "More BUILD.gn fixes")

[commit-bot: I haz the power, 2017-05-23 17:52:47]

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-05-23 17:54:13]

CQ is committing da patch.

Bot data: {"patchset_id": 110001, "attempt_start_ts": 1495561959991170,
"parent_rev": "d37dd4a66e3625714006cf51e6f2efa6154001f2", "commit_rev":
"6803eef1424979ff787aaf4e3b604594f535c384"}

[commit-bot: I haz the power, 2017-05-23 17:54:20]

Description was changed from

==========
Allow embedder to set promise internal field count

Asynchronous context tracking mechanisms in Node.js need to store some
state on all promise objects. This change will allow embedders to
configure the number of internal fields on promises as is already done
for ArrayBuffers.

BUG=v8:6435
==========

to

==========
Allow embedder to set promise internal field count

Asynchronous context tracking mechanisms in Node.js need to store some
state on all promise objects. This change will allow embedders to
configure the number of internal fields on promises as is already done
for ArrayBuffers.

BUG=v8:6435

Review-Url: https://codereview.chromium.org/2889863002
Cr-Commit-Position: refs/heads/master@{#45496}
Committed:
https://chromium.googlesource.com/v8/v8/+/6803eef1424979ff787aaf4e3b604594f53...
==========

[commit-bot: I haz the power, 2017-05-23 17:54:21]

Committed patchset #7 (id:110001) as
https://chromium.googlesource.com/v8/v8/+/6803eef1424979ff787aaf4e3b604594f53...