Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2246333003:
Store information about builtins in descriptors (Closed)
DescriptionStore information about builtins in descriptors
In addition to the code object, store an id, kind, C++ entry point
(where applicable), and debug name for each builtin.
This is a first step towards inlining CPP builtins.
BUG=
Patch Set 1 #Patch Set 2 : Set up correctly after snapshot deserialization #Patch Set 3 : Rename to Builtins::Descriptor #
Total comments: 4
Patch Set 4 : Address comments #
Total comments: 2
Patch Set 5 : Address comments #Patch Set 6 : Fix typo #
Total comments: 5
Patch Set 7 : Convert to static Builtin accessors #Messages
Total messages: 38 (28 generated)
The CQ bit was checked by jgruber@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by jgruber@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by jgruber@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
jgruber@chromium.org changed reviewers: + jarin@chromium.org
Looking good, I have just two comments. https://codereview.chromium.org/2246333003/diff/40001/src/builtins/builtins.cc File src/builtins/builtins.cc (right): https://codereview.chromium.org/2246333003/diff/40001/src/builtins/builtins.c... src/builtins/builtins.cc:128: builtins_[index++].code = code; Instead of just setting the code, should not we construct the descriptor here? (Just like below, but with code set properly.) https://codereview.chromium.org/2246333003/diff/40001/src/builtins/builtins.c... src/builtins/builtins.cc:171: builtins_[index] = {k##Name, API, entry, builtins_[index].code, #Name}; \ Please define a constructor on the Descriptor class and use it here. In the past we had trouble with the initializer lists. Also, as discussed offline, we can set the code field to null here (the deserializer will fill in the right value later).
The CQ bit was checked by jgruber@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks, ptal. https://codereview.chromium.org/2246333003/diff/40001/src/builtins/builtins.cc File src/builtins/builtins.cc (right): https://codereview.chromium.org/2246333003/diff/40001/src/builtins/builtins.c... src/builtins/builtins.cc:128: builtins_[index++].code = code; On 2016/08/18 12:11:18, Jarin wrote: > Instead of just setting the code, should not we construct the descriptor here? > (Just like below, but with code set properly.) Done. https://codereview.chromium.org/2246333003/diff/40001/src/builtins/builtins.c... src/builtins/builtins.cc:171: builtins_[index] = {k##Name, API, entry, builtins_[index].code, #Name}; \ On 2016/08/18 12:11:18, Jarin wrote: > Please define a constructor on the Descriptor class and use it here. In the past > we had trouble with the initializer lists. > > Also, as discussed offline, we can set the code field to null here (the > deserializer will fill in the right value later). Done, and refactored this for more code reuse
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm. Thanks! https://codereview.chromium.org/2246333003/diff/60001/src/builtins/builtins.cc File src/builtins/builtins.cc (right): https://codereview.chromium.org/2246333003/diff/60001/src/builtins/builtins.c... src/builtins/builtins.cc:167: #undef BUILD_ASH For consistency, could you say "#undef MAYBE_CODE" here?
https://codereview.chromium.org/2246333003/diff/60001/src/builtins/builtins.cc File src/builtins/builtins.cc (right): https://codereview.chromium.org/2246333003/diff/60001/src/builtins/builtins.c... src/builtins/builtins.cc:167: #undef BUILD_ASH On 2016/08/18 14:10:43, Jarin wrote: > For consistency, could you say "#undef MAYBE_CODE" here? Good catch, thanks. Done.
The CQ bit was checked by jgruber@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jarin@chromium.org Link to the patchset: https://codereview.chromium.org/2246333003/#ps80001 (title: "Address comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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/22051)
jgruber@chromium.org changed reviewers: + verwaest@chromium.org
+Toon for snapshot/
The CQ bit was checked by jgruber@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
As we discussed offline, I think we can just have a function Address CppBuiltinEntry(BuiltinId id) and tag cpp builtins with Code::CPP_BUILTIN rather than BUILTIN. In the function we can generate a switch that statically returns the addresses. That code can be shared across isolates, unlike the table which needs to be built per isolate. https://codereview.chromium.org/2246333003/diff/100001/src/builtins/builtins.cc File src/builtins/builtins.cc (right): https://codereview.chromium.org/2246333003/diff/100001/src/builtins/builtins.... src/builtins/builtins.cc:174: Code::cast(builtins_[i].code)->set_builtin_index(i); drop cast https://codereview.chromium.org/2246333003/diff/100001/src/builtins/builtins.h File src/builtins/builtins.h (right): https://codereview.chromium.org/2246333003/diff/100001/src/builtins/builtins.... src/builtins/builtins.h:579: enum Kind { CPP, API, TFJ, TFS, ASM, ASH, DBG }; Here I'd write the names out so we don't need to go hunt for their meaning. https://codereview.chromium.org/2246333003/diff/100001/src/builtins/builtins.... src/builtins/builtins.h:599: Object* code; You should be able to just make this a Code*, which is a subclass of Object* https://codereview.chromium.org/2246333003/diff/100001/src/builtins/builtins.... src/builtins/builtins.h:601: const char* debug_name; #ifdef DEBUG? https://codereview.chromium.org/2246333003/diff/100001/src/builtins/builtins.... src/builtins/builtins.h:620: // during the marking phase of mark sweep. See IC::Clear. Then you don't need to reinterpret_cast here.
The CQ bit was checked by jgruber@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux64_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/11229)
Thanks for the reviews everyone. Closing this and inlining it into https://codereview.chromium.org/2259883002/, since it didn't seem to be worth a separate CL anymore.
Description was changed from ========== Store information about builtins in descriptors In addition to the code object, store an id, kind, C++ entry point (where applicable), and debug name for each builtin. This is a first step towards inlining CPP builtins. BUG= ========== to ========== Store information about builtins in descriptors In addition to the code object, store an id, kind, C++ entry point (where applicable), and debug name for each builtin. This is a first step towards inlining CPP builtins. BUG= ========== |
