Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(65)

Issue 1748123003: Rework CallApi*Stubs. (Closed)

Created:
4 years, 9 months ago by vogelheim
Modified:
4 years, 9 months ago
CC:
v8-reviews_googlegroups.com
Base URL:
https://chromium.googlesource.com/v8/v8.git@master
Target Ref:
refs/pending/heads/master
Project:
v8
Visibility:
Public.

Description

Rework CallApi*Stubs. - Eliminate stubs with a variable number of arguments. (That only worked due to their very limited use. These stubs' interface descriptors were basically lying about their number of args, which will fail when used generically.) - Fix all CallApi*Stubs' interface descriptors to no longer lie about their arguments. - Unify CallApi*Stub, for * in Function, Accessor, FunctionWithFixedArgs. (Since these are now all doing the same thing.) - Rename the unified stub (and interface descriptors) to *ApiCallback*, since that's really what they're doing. - Refuse inlining an API callback if its number of parameters exceeds the supported number of args. BUG= Committed: https://crrev.com/d238b953a474272c0e3ea22ef6a9b63fa9729340 Cr-Commit-Position: refs/heads/master@{#34614} Committed: https://crrev.com/5096492f1b0460b2644c7f79f9ccc5881fc65c50 Cr-Commit-Position: refs/heads/master@{#34627}

Patch Set 1 : #

Total comments: 5

Patch Set 2 : Feedback. #

Patch Set 3 : Rebase. #

Patch Set 4 : And BAAM!! All the platforms. #

Patch Set 5 : Minor fixes and make Win compiler happy. #

Total comments: 2
Unified diffs Side-by-side diffs Delta from patch set Stats (+332 lines, -763 lines) Patch
M src/arm/code-stubs-arm.cc View 1 2 3 6 chunks +16 lines, -54 lines 0 comments Download
M src/arm/interface-descriptors-arm.cc View 1 2 3 1 chunk +1 line, -19 lines 0 comments Download
M src/arm64/code-stubs-arm64.cc View 1 2 3 6 chunks +13 lines, -51 lines 0 comments Download
M src/arm64/interface-descriptors-arm64.cc View 1 2 3 1 chunk +1 line, -19 lines 0 comments Download
M src/code-stubs.h View 1 2 3 chunks +21 lines, -53 lines 0 comments Download
M src/compiler/fast-accessor-assembler.cc View 1 2 3 3 chunks +13 lines, -15 lines 2 comments Download
M src/crankshaft/hydrogen.cc View 1 2 3 4 2 chunks +10 lines, -17 lines 0 comments Download
M src/ia32/code-stubs-ia32.cc View 1 2 3 8 chunks +16 lines, -61 lines 0 comments Download
M src/ia32/interface-descriptors-ia32.cc View 1 2 3 1 chunk +1 line, -15 lines 0 comments Download
M src/ic/arm/handler-compiler-arm.cc View 1 2 3 2 chunks +2 lines, -2 lines 0 comments Download
M src/ic/arm64/handler-compiler-arm64.cc View 1 2 3 2 chunks +2 lines, -2 lines 0 comments Download
M src/ic/ia32/handler-compiler-ia32.cc View 1 2 3 2 chunks +2 lines, -2 lines 0 comments Download
M src/ic/mips/handler-compiler-mips.cc View 1 2 3 2 chunks +2 lines, -2 lines 0 comments Download
M src/ic/mips64/handler-compiler-mips64.cc View 1 2 3 2 chunks +2 lines, -2 lines 0 comments Download
M src/ic/ppc/handler-compiler-ppc.cc View 1 2 3 2 chunks +2 lines, -2 lines 0 comments Download
M src/ic/s390/handler-compiler-s390.cc View 1 2 3 2 chunks +2 lines, -2 lines 0 comments Download
M src/ic/x64/handler-compiler-x64.cc View 1 2 3 2 chunks +2 lines, -2 lines 0 comments Download
M src/ic/x87/handler-compiler-x87.cc View 1 2 3 2 chunks +2 lines, -2 lines 0 comments Download
M src/interface-descriptors.h View 1 2 3 4 7 chunks +95 lines, -16 lines 0 comments Download
M src/interface-descriptors.cc View 1 2 3 4 1 chunk +31 lines, -14 lines 0 comments Download
M src/mips/code-stubs-mips.cc View 1 2 3 5 chunks +14 lines, -54 lines 0 comments Download
M src/mips/interface-descriptors-mips.cc View 1 2 3 1 chunk +1 line, -15 lines 0 comments Download
M src/mips64/code-stubs-mips64.cc View 1 2 3 5 chunks +17 lines, -58 lines 0 comments Download
M src/mips64/interface-descriptors-mips64.cc View 1 2 3 1 chunk +1 line, -15 lines 0 comments Download
M src/ppc/code-stubs-ppc.cc View 1 2 3 6 chunks +15 lines, -51 lines 0 comments Download
M src/ppc/interface-descriptors-ppc.cc View 1 2 3 1 chunk +1 line, -15 lines 0 comments Download
M src/s390/code-stubs-s390.cc View 1 2 3 6 chunks +14 lines, -50 lines 0 comments Download
M src/s390/interface-descriptors-s390.cc View 1 2 3 1 chunk +1 line, -13 lines 0 comments Download
M src/x64/code-stubs-x64.cc View 1 2 6 chunks +14 lines, -51 lines 0 comments Download
M src/x64/interface-descriptors-x64.cc View 1 chunk +1 line, -15 lines 0 comments Download
M src/x87/code-stubs-x87.cc View 1 2 3 8 chunks +16 lines, -59 lines 0 comments Download
M src/x87/interface-descriptors-x87.cc View 1 2 3 1 chunk +1 line, -15 lines 0 comments Download

Messages

Total messages: 38 (18 generated)
vogelheim
Please have a look. This somehow became more elaborate than I had hoped; but at ...
4 years, 9 months ago (2016-03-01 17:00:49 UTC) #4
danno
https://codereview.chromium.org/1748123003/diff/20001/src/interface-descriptors.cc File src/interface-descriptors.cc (right): https://codereview.chromium.org/1748123003/diff/20001/src/interface-descriptors.cc#newcode515 src/interface-descriptors.cc:515: return ApiCallbackWith7ArgsDescriptor(isolate); Shouldn't you have a default case that ...
4 years, 9 months ago (2016-03-04 18:07:43 UTC) #5
vogelheim
ptal https://codereview.chromium.org/1748123003/diff/20001/src/interface-descriptors.cc File src/interface-descriptors.cc (right): https://codereview.chromium.org/1748123003/diff/20001/src/interface-descriptors.cc#newcode515 src/interface-descriptors.cc:515: return ApiCallbackWith7ArgsDescriptor(isolate); On 2016/03/04 18:07:42, danno wrote: > ...
4 years, 9 months ago (2016-03-08 12:59:27 UTC) #6
danno
lgtm https://codereview.chromium.org/1748123003/diff/20001/src/interface-descriptors.cc File src/interface-descriptors.cc (right): https://codereview.chromium.org/1748123003/diff/20001/src/interface-descriptors.cc#newcode515 src/interface-descriptors.cc:515: return ApiCallbackWith7ArgsDescriptor(isolate); On 2016/03/08 12:59:27, vogelheim wrote: > ...
4 years, 9 months ago (2016-03-09 10:23:38 UTC) #7
vogelheim
https://codereview.chromium.org/1748123003/diff/100001/src/compiler/fast-accessor-assembler.cc File src/compiler/fast-accessor-assembler.cc (right): https://codereview.chromium.org/1748123003/diff/100001/src/compiler/fast-accessor-assembler.cc#newcode197 src/compiler/fast-accessor-assembler.cc:197: assembler_->UndefinedConstant()}; On 2016/03/09 10:23:38, danno wrote: > It might ...
4 years, 9 months ago (2016-03-09 11:09:35 UTC) #8
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1748123003/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1748123003/100001
4 years, 9 months ago (2016-03-09 11:10:03 UTC) #10
commit-bot: I haz the power
Dry run: This issue passed the CQ dry run.
4 years, 9 months ago (2016-03-09 11:11:50 UTC) #12
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1748123003/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1748123003/100001
4 years, 9 months ago (2016-03-09 11:48:37 UTC) #14
commit-bot: I haz the power
Try jobs failed on following builders: v8_presubmit on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/12081)
4 years, 9 months ago (2016-03-09 11:51:28 UTC) #16
Jakob Kummerow
src/ic/* LGTM.
4 years, 9 months ago (2016-03-09 12:11:06 UTC) #18
Michael Starzinger
LGTM stamp on "compiler", didn't look at the rest.
4 years, 9 months ago (2016-03-09 12:15:43 UTC) #20
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1748123003/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1748123003/100001
4 years, 9 months ago (2016-03-09 12:16:24 UTC) #22
commit-bot: I haz the power
Committed patchset #5 (id:100001)
4 years, 9 months ago (2016-03-09 12:18:09 UTC) #24
commit-bot: I haz the power
Patchset 5 (id:??) landed as https://crrev.com/d238b953a474272c0e3ea22ef6a9b63fa9729340 Cr-Commit-Position: refs/heads/master@{#34614}
4 years, 9 months ago (2016-03-09 12:18:22 UTC) #26
vogelheim
A revert of this CL (patchset #5 id:100001) has been created in https://codereview.chromium.org/1775933005/ by vogelheim@chromium.org. ...
4 years, 9 months ago (2016-03-09 14:58:23 UTC) #27
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1748123003/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1748123003/100001
4 years, 9 months ago (2016-03-09 15:49:48 UTC) #30
commit-bot: I haz the power
Dry run: This issue passed the CQ dry run.
4 years, 9 months ago (2016-03-09 16:10:20 UTC) #32
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1748123003/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1748123003/100001
4 years, 9 months ago (2016-03-09 16:10:47 UTC) #34
commit-bot: I haz the power
Committed patchset #5 (id:100001)
4 years, 9 months ago (2016-03-09 16:18:34 UTC) #36
commit-bot: I haz the power
4 years, 9 months ago (2016-03-09 16:19:14 UTC) #38
Message was sent while issue was closed.
Patchset 5 (id:??) landed as
https://crrev.com/5096492f1b0460b2644c7f79f9ccc5881fc65c50
Cr-Commit-Position: refs/heads/master@{#34627}

Powered by Google App Engine
This is Rietveld 408576698