Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(9)

Issue 2497523002: [promises] Move promise constructor to TFS (Closed)

Created:
2 years, 10 months ago by gsathya
Modified:
2 years, 9 months ago
CC:
rmcilroy, adamk
Target Ref:
refs/pending/heads/master
Project:
v8
Visibility:
Public.

Description

[promises] Move promise constructor to TFS BUG=v8:5343, chromium:660947, chromium:658194 Committed: https://crrev.com/b361b59fffe5480ebd3eacda9d4a4274d43f8a95 Cr-Commit-Position: refs/heads/master@{#41438}

Patch Set 1 #

Patch Set 2 : remove temp code #

Total comments: 20

Patch Set 3 : create internal promise #

Patch Set 4 : add ispromise #

Patch Set 5 : remove todo #

Patch Set 6 : add asm wrapper #

Patch Set 7 : add fast path #

Patch Set 8 : more fixes #

Patch Set 9 : add catch prediction #

Patch Set 10 : fix tests #

Patch Set 11 : remove unused var #

Patch Set 12 : do exact check #

Patch Set 13 : fix test #

Patch Set 14 : fixees #

Total comments: 3

Patch Set 15 : use jsbuiltinconstruct stub #

Total comments: 22

Patch Set 16 : fixes #

Patch Set 17 : more fixes #

Total comments: 1

Patch Set 18 : add goto #

Total comments: 2

Patch Set 19 : fix test #

Total comments: 17

Patch Set 20 : fix nits #

Unified diffs Side-by-side diffs Delta from patch set Stats (+309 lines, -105 lines) Patch
M src/bootstrapper.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 2 chunks +70 lines, -40 lines 0 comments Download
M src/builtins/builtins.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 1 chunk +3 lines, -0 lines 0 comments Download
M src/builtins/builtins-promise.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 2 chunks +159 lines, -0 lines 0 comments Download
M src/contexts.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 1 chunk +30 lines, -27 lines 0 comments Download
M src/isolate.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 1 chunk +4 lines, -0 lines 0 comments Download
M src/js/promise.js View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 6 chunks +5 lines, -37 lines 0 comments Download
M src/objects.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 3 chunks +9 lines, -1 line 0 comments Download
M src/objects-inl.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 1 chunk +13 lines, -0 lines 0 comments Download
M src/runtime/runtime.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 1 chunk +1 line, -0 lines 0 comments Download
M src/runtime/runtime-promise.cc View 1 2 1 chunk +15 lines, -0 lines 0 comments Download

Messages

Total messages: 79 (59 generated)
gsathya
https://codereview.chromium.org/2497523002/diff/20001/src/builtins/builtins-promise.cc File src/builtins/builtins-promise.cc (right): https://codereview.chromium.org/2497523002/diff/20001/src/builtins/builtins-promise.cc#newcode148 src/builtins/builtins-promise.cc:148: a->CallRuntime(Runtime::kCreateResolvingFunctions, context, instance); Is there a way to call ...
2 years, 10 months ago (2016-11-11 02:48:18 UTC) #3
Benedikt Meurer
First round of comments. Adding jgruber@ since he did the regexp.js recently, so he has ...
2 years, 10 months ago (2016-11-11 06:50:31 UTC) #5
jgruber
Nice! Just a couple of nits from me. https://codereview.chromium.org/2497523002/diff/20001/src/builtins/builtins-promise.cc File src/builtins/builtins-promise.cc (right): https://codereview.chromium.org/2497523002/diff/20001/src/builtins/builtins-promise.cc#newcode94 src/builtins/builtins-promise.cc:94: Variable ...
2 years, 10 months ago (2016-11-11 08:05:19 UTC) #6
jgruber
https://codereview.chromium.org/2497523002/diff/20001/src/builtins/builtins-promise.cc File src/builtins/builtins-promise.cc (right): https://codereview.chromium.org/2497523002/diff/20001/src/builtins/builtins-promise.cc#newcode127 src/builtins/builtins-promise.cc:127: Callable callable = CodeFactory::ToString(isolate); Danno added ToString to CSA ...
2 years, 10 months ago (2016-11-11 09:25:43 UTC) #7
gsathya
PTAL, will fix the other backends as well, but wanted to get thoughts on the ...
2 years, 10 months ago (2016-11-22 04:31:44 UTC) #32
Benedikt Meurer
https://codereview.chromium.org/2497523002/diff/260001/src/builtins/x64/builtins-x64.cc File src/builtins/x64/builtins-x64.cc (right): https://codereview.chromium.org/2497523002/diff/260001/src/builtins/x64/builtins-x64.cc#newcode2842 src/builtins/x64/builtins-x64.cc:2842: void Builtins::Generate_PromiseConstructorHelper(MacroAssembler* masm) { Unless I'm missing something, this ...
2 years, 10 months ago (2016-11-22 06:03:28 UTC) #35
gsathya
On 2016/11/22 06:03:28, Benedikt Meurer wrote: > https://codereview.chromium.org/2497523002/diff/260001/src/builtins/x64/builtins-x64.cc > File src/builtins/x64/builtins-x64.cc (right): > > https://codereview.chromium.org/2497523002/diff/260001/src/builtins/x64/builtins-x64.cc#newcode2842 ...
2 years, 10 months ago (2016-11-22 06:38:25 UTC) #38
jgruber
https://codereview.chromium.org/2497523002/diff/280001/src/bootstrapper.cc File src/bootstrapper.cc (right): https://codereview.chromium.org/2497523002/diff/280001/src/bootstrapper.cc#newcode1818 src/bootstrapper.cc:1818: shared->set_instance_class_name(isolate->heap()->Object_string()); Shouldn't the instance class name be 'Promise'? If ...
2 years, 10 months ago (2016-11-22 08:55:56 UTC) #42
gsathya
https://codereview.chromium.org/2497523002/diff/280001/src/bootstrapper.cc File src/bootstrapper.cc (right): https://codereview.chromium.org/2497523002/diff/280001/src/bootstrapper.cc#newcode1818 src/bootstrapper.cc:1818: shared->set_instance_class_name(isolate->heap()->Object_string()); On 2016/11/22 08:55:55, jgruber wrote: > Shouldn't the ...
2 years, 10 months ago (2016-11-23 14:31:18 UTC) #45
jgruber
Very nice, lgtm modulo test failures. https://codereview.chromium.org/2497523002/diff/340001/src/builtins/builtins-promise.cc File src/builtins/builtins-promise.cc (right): https://codereview.chromium.org/2497523002/diff/340001/src/builtins/builtins-promise.cc#newcode133 src/builtins/builtins-promise.cc:133: Callable fast_new_object_stub = ...
2 years, 9 months ago (2016-11-24 07:42:30 UTC) #52
gsathya
On 2016/11/24 07:42:30, jgruber wrote: > Very nice, lgtm modulo test failures. > > https://codereview.chromium.org/2497523002/diff/340001/src/builtins/builtins-promise.cc ...
2 years, 9 months ago (2016-11-24 08:21:34 UTC) #55
Benedikt Meurer
LGTM from my side. Let Igor have a final look.
2 years, 9 months ago (2016-11-28 09:10:40 UTC) #60
caitp
some drive-by ideas: https://codereview.chromium.org/2497523002/diff/360001/src/builtins/builtins-promise.cc File src/builtins/builtins-promise.cc (right): https://codereview.chromium.org/2497523002/diff/360001/src/builtins/builtins-promise.cc#newcode236 src/builtins/builtins-promise.cc:236: &if_ispromise, &if_isnotpromise); Why not just `a.Branch(a.HasInstanceType(maybe_promise, ...
2 years, 9 months ago (2016-11-29 13:33:01 UTC) #62
Igor Sheludko
Mostly nits. lgtm once IsDebugActive() is addressed. I'm sorry for the delay. https://codereview.chromium.org/2497523002/diff/360001/src/builtins/builtins-promise.cc File src/builtins/builtins-promise.cc ...
2 years, 9 months ago (2016-12-01 09:25:31 UTC) #64
gsathya
Thanks for the reviews. caitp, PTAL https://codereview.chromium.org/2497523002/diff/360001/src/builtins/builtins-promise.cc File src/builtins/builtins-promise.cc (right): https://codereview.chromium.org/2497523002/diff/360001/src/builtins/builtins-promise.cc#newcode98 src/builtins/builtins-promise.cc:98: a.GotoIf(a.WordEqual(new_target, a.UndefinedConstant()), On ...
2 years, 9 months ago (2016-12-01 20:03:53 UTC) #67
caitp
https://codereview.chromium.org/2497523002/diff/360001/src/builtins/builtins-promise.cc File src/builtins/builtins-promise.cc (right): https://codereview.chromium.org/2497523002/diff/360001/src/builtins/builtins-promise.cc#newcode236 src/builtins/builtins-promise.cc:236: &if_ispromise, &if_isnotpromise); On 2016/12/01 20:03:53, gsathya wrote: > On ...
2 years, 9 months ago (2016-12-01 20:12:38 UTC) #68
gsathya
On 2016/12/01 20:12:38, caitp wrote: > https://codereview.chromium.org/2497523002/diff/360001/src/builtins/builtins-promise.cc > File src/builtins/builtins-promise.cc (right): > > https://codereview.chromium.org/2497523002/diff/360001/src/builtins/builtins-promise.cc#newcode236 > ...
2 years, 9 months ago (2016-12-01 21:07:21 UTC) #71
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2497523002/380001
2 years, 9 months ago (2016-12-01 21:07:49 UTC) #74
commit-bot: I haz the power
Committed patchset #20 (id:380001)
2 years, 9 months ago (2016-12-01 21:09:53 UTC) #77
commit-bot: I haz the power
2 years, 9 months ago (2016-12-01 21:10:12 UTC) #79
Message was sent while issue was closed.
Patchset 20 (id:??) landed as
https://crrev.com/b361b59fffe5480ebd3eacda9d4a4274d43f8a95
Cr-Commit-Position: refs/heads/master@{#41438}

Powered by Google App Engine
This is Rietveld 408576698