Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1474543004:
Implement Fast Accessor Builder (Closed)
|
Created:
5 years ago by vogelheim Modified:
5 years ago Reviewers:
epertoso, jochen (gone - plz use gerrit), Michael Starzinger, Michael Achenbach, Benedikt Meurer, mvstanton CC:
Paweł Hajdan Jr., v8-reviews_googlegroups.com, tandrii(chromium), Sergiy Byelozyorov Base URL:
https://chromium.googlesource.com/v8/v8.git@master Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
DescriptionImplement FastAccessorBuilder.
... using the RawMachineAssembler and the work in cl/1407313004
BUG=chromium:508898
LOG=Y
Committed: https://crrev.com/515d9ccd8e6df7bf2ca01e2a55aaad30226399e1
Cr-Commit-Position: refs/heads/master@{#32742}
Patch Set 1 #
Total comments: 11
Patch Set 2 : Rebase + remove Enrico's changes. #Patch Set 3 : A more complete implementation. #Patch Set 4 : Further cleanup #Patch Set 5 : Review feedback: Seperate types for value + label ids. #
Total comments: 30
Patch Set 6 : Review feedback; all of the easy stuff. #
Total comments: 1
Patch Set 7 : Review feedback: split headers; forward declare stuff. #Patch Set 8 : Add tests; remove GetParameters. #Patch Set 9 : Add tests for load ops. #
Total comments: 17
Patch Set 10 : Review feedback. #
Total comments: 8
Patch Set 11 : Review feedback. #
Total comments: 14
Patch Set 12 : Review feedback. #Patch Set 13 : Rebase. #Patch Set 14 : Remove impl_ cleanup. Drop tests for --optimize-for-size. Also rebase. #
Messages
Total messages: 45 (12 generated)
Please, update this so that it shows just your changes.
jochen@chromium.org changed reviewers: + epertoso@chromium.org, jochen@chromium.org
epertoso@chromium.org changed reviewers: - epertoso@chromium.org, jochen@chromium.org
https://codereview.chromium.org/1474543004/diff/1/src/compiler/fast-accessor-... File src/compiler/fast-accessor-assembler.cc (right): https://codereview.chromium.org/1474543004/diff/1/src/compiler/fast-accessor-... src/compiler/fast-accessor-assembler.cc:76: nodes_.clear(); These two calls to clear() don't do what you seem to imply they're doing. C++11 standard, [sequence.reqmts], doesn't require the container to free up the backing storage. https://codereview.chromium.org/1474543004/diff/1/src/compiler/fast-accessor-... src/compiler/fast-accessor-assembler.cc:82: Handle<Code> code = Pipeline::GenerateCodeForTesting( nit: add a TODO here, we should probably not use a 'ForTesting' method. https://codereview.chromium.org/1474543004/diff/1/src/compiler/fast-accessor-... src/compiler/fast-accessor-assembler.cc:106: nodes_.resize(labels_.size(), nullptr); Why? https://codereview.chromium.org/1474543004/diff/1/src/compiler/fast-accessor-... File src/compiler/fast-accessor-assembler.h (right): https://codereview.chromium.org/1474543004/diff/1/src/compiler/fast-accessor-... src/compiler/fast-accessor-assembler.h:45: typedef size_t ValueId; vector<Node*>::size_type would be more accurate. https://codereview.chromium.org/1474543004/diff/1/src/compiler/fast-accessor-... src/compiler/fast-accessor-assembler.h:50: // Builder / assembler functions: GetInternalField, LoadObject, CheckFlagsSetOrReturnNull, SetLabel, ChecNotNullOrJump are not implemented. https://codereview.chromium.org/1474543004/diff/1/src/compiler/fast-accessor-... src/compiler/fast-accessor-assembler.h:78: ValueId FromLabel(Label* label); FromLabel and LabelFromValue are not implemented. https://codereview.chromium.org/1474543004/diff/1/src/compiler/fast-accessor-... src/compiler/fast-accessor-assembler.h:93: std::vector<Label*> labels_; You're never using labels_, are you? https://codereview.chromium.org/1474543004/diff/1/test/cctest/test-api-fast-a... File test/cctest/test-api-fast-accessor-builder.cc (right): https://codereview.chromium.org/1474543004/diff/1/test/cctest/test-api-fast-a... test/cctest/test-api-fast-accessor-builder.cc:1: // Copyright 2015 the V8 project authors. All rights reserved. Could you actually merge this test with test-api-accessors? Or, even better, you can just test the builder here, and use it in test-api-accessors.cc.
https://codereview.chromium.org/1474543004/diff/1/src/compiler/fast-accessor-... File src/compiler/fast-accessor-assembler.cc (right): https://codereview.chromium.org/1474543004/diff/1/src/compiler/fast-accessor-... src/compiler/fast-accessor-assembler.cc:76: nodes_.clear(); On 2015/11/25 10:31:15, epertoso wrote: > These two calls to clear() don't do what you seem to imply they're doing. > > C++11 standard, [sequence.reqmts], doesn't require the container to free up the > backing storage. You're technically correct... :-) Well.. I'm trying to enforce the 'two stage' life cycle of the object: While building up, I require nodes_ + labels_ to match the API-visible ValueIds to the internal Node*. At this stage, I no longer need them, and don't want the code to access them any longer. https://codereview.chromium.org/1474543004/diff/1/src/compiler/fast-accessor-... src/compiler/fast-accessor-assembler.cc:82: Handle<Code> code = Pipeline::GenerateCodeForTesting( On 2015/11/25 10:31:15, epertoso wrote: > nit: add a TODO here, we should probably not use a 'ForTesting' method. Yes. src/compiler/* owners will have an opinion on this before it gets submitted... https://codereview.chromium.org/1474543004/diff/1/src/compiler/fast-accessor-... src/compiler/fast-accessor-assembler.cc:106: nodes_.resize(labels_.size(), nullptr); On 2015/11/25 10:31:15, epertoso wrote: > Why? So... I'm trying to hide the RawMachineAssembler implementation, which is why I present ValueId integers at the API. I need nodes_ + labels_ to match up the API-visible types to internal types. Since I have only one type externally - everything is a ValueId - but two internally - Label + Node*, I somehow need to disambiguate them. So the idea is that for any given ValueId n, either label_[n] or nodes_[n] is non-null, but not both. Since none of the label stuff is implemented here, that was admittedly a bit hard to guess.
Description was changed from ========== [For discussion; not really ready for review.] BUG= ========== to ========== Implement FastAccessorBuilder. ... using the RawMachineAssembler and the work in cl/1407313004 BUG=chromium:508898 ==========
vogelheim@chromium.org changed reviewers: + epertoso@chromium.org, jochen@chromium.org, mstarzinger@chromium.org
Please have a look.
This isn't quite complete yet, but I think it's far enough along that I'd like
some feedback.
What's missing:
- A more complete set of unit tests.
- The 'native callback' is missing. But since RMA supports C function callbacks,
I don't expect that to be that much work.
- Maintainability:
- I think I should split out my changes from api.cc.
- I'm unhappy with including rma header in fast-accessor-assembler.h, since
that way I de-facto export that other header in places where it isn't supposed
to go.
(It might be best to fix this in a separate CL.)
https://codereview.chromium.org/1474543004/diff/80001/include/v8.h File include/v8.h (right): https://codereview.chromium.org/1474543004/diff/80001/include/v8.h#newcode6982 include/v8.h:6982: namespace experimental { i wonder whether we should move this to a separate header? https://codereview.chromium.org/1474543004/diff/80001/include/v8.h#newcode6986 include/v8.h:6986: struct ValueId { why not using size_t = ValudId or typedef? https://codereview.chromium.org/1474543004/diff/80001/include/v8.h#newcode7012 include/v8.h:7012: }; disallow copy/assign https://codereview.chromium.org/1474543004/diff/80001/src/DEPS File src/DEPS (right): https://codereview.chromium.org/1474543004/diff/80001/src/DEPS#newcode5 src/DEPS:5: "+src/compiler/fast-accessor-assembler.h", maybe make this a per-file rule, restricting it to api.cc? https://codereview.chromium.org/1474543004/diff/80001/test/cctest/test-api-fa... File test/cctest/test-api-fast-accessor-builder.cc (right): https://codereview.chromium.org/1474543004/diff/80001/test/cctest/test-api-fa... test/cctest/test-api-fast-accessor-builder.cc:6: #define V8_IMMINENT_DEPRECATION_WARNINGS yay :P
https://codereview.chromium.org/1474543004/diff/80001/include/v8.h File include/v8.h (right): https://codereview.chromium.org/1474543004/diff/80001/include/v8.h#newcode6986 include/v8.h:6986: struct ValueId { On 2015/11/27 at 08:33:45, jochen wrote: > why not using size_t = ValudId or typedef? It was my suggestion. We don't want to pass ValueIds instead of LabelIds. Typedef is not strong enough.
https://codereview.chromium.org/1474543004/diff/80001/include/v8.h File include/v8.h (right): https://codereview.chromium.org/1474543004/diff/80001/include/v8.h#newcode6982 include/v8.h:6982: namespace experimental { On 2015/11/27 08:33:45, jochen wrote: > i wonder whether we should move this to a separate header? +1 https://codereview.chromium.org/1474543004/diff/80001/src/compiler/fast-acces... File src/compiler/fast-accessor-assembler.cc (right): https://codereview.chromium.org/1474543004/diff/80001/src/compiler/fast-acces... src/compiler/fast-accessor-assembler.cc:25: Linkage::GetJSCallDescriptor(&zone_, false, 1, Not sure if a JavaScript call descriptor is what we want here in the long run. IIUC these accessors do not represent JavaScript functions and hence will not follow their calling convention. https://codereview.chromium.org/1474543004/diff/80001/src/compiler/fast-acces... File src/compiler/fast-accessor-assembler.h (right): https://codereview.chromium.org/1474543004/diff/80001/src/compiler/fast-acces... src/compiler/fast-accessor-assembler.h:13: #include "src/compiler/raw-machine-assembler.h" I understand that RMA::Label still forces us to include this header, but as discussed offline, we could avoid this by making "Label" not be an inner class. Can we leave a TODO here in that regard? Also, since this header is allowed to be included from the outside, it should contain the following comment (similar to pipeline.h) in front of the includes: // Clients of this interface shouldn't depend on lots of compiler internals. // Do not include anything from src/compiler here! https://codereview.chromium.org/1474543004/diff/80001/src/compiler/fast-acces... src/compiler/fast-accessor-assembler.h:32: // use by the API to implement Fast Dom Accessors. (crbug.com/508898). nit: Let's drop the reference to the "crbug" in the comment. https://codereview.chromium.org/1474543004/diff/80001/test/cctest/test-api-fa... File test/cctest/test-api-fast-accessor-builder.cc (right): https://codereview.chromium.org/1474543004/diff/80001/test/cctest/test-api-fa... test/cctest/test-api-fast-accessor-builder.cc:55: ExpectInt32("f = new foo(); f.barf", 124); This test seems to assume that it takes exactly four runs to trigger the "fast" accessor. I don't know the exact details behind this, but this looks fragile. Can this be made more robust?
bmeurer@chromium.org changed reviewers: + bmeurer@chromium.org
https://codereview.chromium.org/1474543004/diff/80001/src/compiler/fast-acces... File src/compiler/fast-accessor-assembler.h (right): https://codereview.chromium.org/1474543004/diff/80001/src/compiler/fast-acces... src/compiler/fast-accessor-assembler.h:13: #include "src/compiler/raw-machine-assembler.h" How about making RMA::Label forward declarable first? https://codereview.chromium.org/1474543004/diff/80001/src/compiler/fast-acces... src/compiler/fast-accessor-assembler.h:97: enum { BUILDING, BUILT, ERROR } state_; Nit: Can we follow the style guide and use kBuilding, kBuilt and kError here?
epertoso@chromium.org changed reviewers: + mvstanton@chromium.org
https://codereview.chromium.org/1474543004/diff/80001/src/compiler/fast-acces... File src/compiler/fast-accessor-assembler.cc (right): https://codereview.chromium.org/1474543004/diff/80001/src/compiler/fast-acces... src/compiler/fast-accessor-assembler.cc:25: Linkage::GetJSCallDescriptor(&zone_, false, 1, On 2015/11/27 at 09:15:52, Michael Starzinger wrote: > Not sure if a JavaScript call descriptor is what we want here in the long run. IIUC these accessors do not represent JavaScript functions and hence will not follow their calling convention. Currently we just tail-call these accessors, and in this way we don't have to move the arguments around. See http://crrev.com/1407313004. If you have a strong reason to use another call descriptor, I'm going to modify the builtins and the handler compiler as well. https://codereview.chromium.org/1474543004/diff/80001/src/compiler/fast-acces... File src/compiler/fast-accessor-assembler.h (right): https://codereview.chromium.org/1474543004/diff/80001/src/compiler/fast-acces... src/compiler/fast-accessor-assembler.h:13: #include "src/compiler/raw-machine-assembler.h" On 2015/11/27 at 09:26:23, Benedikt Meurer wrote: > How about making RMA::Label forward declarable first? +1. https://codereview.chromium.org/1474543004/diff/80001/test/cctest/test-api-fa... File test/cctest/test-api-fast-accessor-builder.cc (right): https://codereview.chromium.org/1474543004/diff/80001/test/cctest/test-api-fa... test/cctest/test-api-fast-accessor-builder.cc:55: ExpectInt32("f = new foo(); f.barf", 124); On 2015/11/27 at 09:15:53, Michael Starzinger wrote: > This test seems to assume that it takes exactly four runs to trigger the "fast" accessor. I don't know the exact details behind this, but this looks fragile. Can this be made more robust? +mvstanton.
DBC to answer a query.. https://codereview.chromium.org/1474543004/diff/80001/test/cctest/test-api-fa... File test/cctest/test-api-fast-accessor-builder.cc (right): https://codereview.chromium.org/1474543004/diff/80001/test/cctest/test-api-fa... test/cctest/test-api-fast-accessor-builder.cc:55: ExpectInt32("f = new foo(); f.barf", 124); On 2015/11/27 09:15:53, Michael Starzinger wrote: > This test seems to assume that it takes exactly four runs to trigger the "fast" > accessor. I don't know the exact details behind this, but this looks fragile. > Can this be made more robust? Good point. The code has the problem that it works accidentally. I read it and wondered how in the heck are we ratcheting up the IC state chain? It's because there is code/compilation caching going on for the same input strings. That's a fragile assumption. I'd much prefer it to be written like: CompileRun("function bar(f) { return f.barf; }"); and then ExpectInt32("bar(new foo());", 123); // PREMONOMORPHIC ExpectInt32("bar(new foo());", 123); // MONOMORPHIC ExpectInt32("bar(new foo());", 124); // MONOMORPHIC handler called. That way it's clear that you are cycling the load through IC states. And the comment shows why you expect it to take until the third call to get the right answer. Also...it really should be on the third call, not the 4th, right? I suspect it's the 4th because of the implementation of the aforementioned code caching.
https://codereview.chromium.org/1474543004/diff/80001/test/cctest/test-api-fa... File test/cctest/test-api-fast-accessor-builder.cc (right): https://codereview.chromium.org/1474543004/diff/80001/test/cctest/test-api-fa... test/cctest/test-api-fast-accessor-builder.cc:55: ExpectInt32("f = new foo(); f.barf", 124); On 2015/11/27 10:21:17, mvstanton wrote: > On 2015/11/27 09:15:53, Michael Starzinger wrote: > > This test seems to assume that it takes exactly four runs to trigger the > "fast" > > accessor. I don't know the exact details behind this, but this looks fragile. > > Can this be made more robust? > > Good point. The code has the problem that it works accidentally. I read it and > wondered how in the heck are we ratcheting up the IC state chain? It's because > there is code/compilation caching going on for the same input strings. That's a > fragile assumption. I'd much prefer it to be written like: > > CompileRun("function bar(f) { return f.barf; }"); > > and then > > ExpectInt32("bar(new foo());", 123); // PREMONOMORPHIC > ExpectInt32("bar(new foo());", 123); // MONOMORPHIC > ExpectInt32("bar(new foo());", 124); // MONOMORPHIC handler called. > > That way it's clear that you are cycling the load through IC states. > > And the comment shows why you expect it to take until the third call to get the > right answer. > > Also...it really should be on the third call, not the 4th, right? I suspect it's > the 4th because of the implementation of the aforementioned code caching. SGTM. I am fine with wrapping it in a function and with the comments that Michael suggested. Avoiding cycling through the IC states would probably be too cumbersome.
Description was changed from ========== Implement FastAccessorBuilder. ... using the RawMachineAssembler and the work in cl/1407313004 BUG=chromium:508898 ========== to ========== Implement FastAccessorBuilder. ... using the RawMachineAssembler and the work in cl/1407313004 BUG=chromium:508898 LOG=Y ==========
Did the easy stuff. Still need to split files & finish the unit tests. Also, there's unresolved issue that the new way of warm-upping the function for unit tests doesn't work with --always-opt. Enrico has offered to look into that. https://codereview.chromium.org/1474543004/diff/80001/include/v8.h File include/v8.h (right): https://codereview.chromium.org/1474543004/diff/80001/include/v8.h#newcode6986 include/v8.h:6986: struct ValueId { On 2015/11/27 09:13:13, epertoso wrote: > On 2015/11/27 at 08:33:45, jochen wrote: > > why not using size_t = ValudId or typedef? > > It was my suggestion. We don't want to pass ValueIds instead of LabelIds. > Typedef is not strong enough. ^^^ This. The original version used a single typedef for both, and then dis-ambiguated this internally. Alternatively, we could use a typedef to the same type, but since in C++ those are mutually assignable, that's not very nice to use. Among those alternatives, I like the current version the best. Let me know if you disagree strongly. https://codereview.chromium.org/1474543004/diff/80001/include/v8.h#newcode7012 include/v8.h:7012: }; On 2015/11/27 08:33:45, jochen wrote: > disallow copy/assign Done. v8.h can't include macros.h, so done manually. https://codereview.chromium.org/1474543004/diff/80001/src/DEPS File src/DEPS (right): https://codereview.chromium.org/1474543004/diff/80001/src/DEPS#newcode5 src/DEPS:5: "+src/compiler/fast-accessor-assembler.h", On 2015/11/27 08:33:45, jochen wrote: > maybe make this a per-file rule, restricting it to api.cc? Done. https://codereview.chromium.org/1474543004/diff/80001/src/compiler/fast-acces... File src/compiler/fast-accessor-assembler.cc (right): https://codereview.chromium.org/1474543004/diff/80001/src/compiler/fast-acces... src/compiler/fast-accessor-assembler.cc:25: Linkage::GetJSCallDescriptor(&zone_, false, 1, On 2015/11/27 10:04:56, epertoso wrote: > On 2015/11/27 at 09:15:52, Michael Starzinger wrote: > > Not sure if a JavaScript call descriptor is what we want here in the long run. > IIUC these accessors do not represent JavaScript functions and hence will not > follow their calling convention. > > Currently we just tail-call these accessors, and in this way we don't have to > move the arguments around. See http://crrev.com/1407313004. > > If you have a strong reason to use another call descriptor, I'm going to modify > the builtins and the handler compiler as well. Ack to both comments. As long as we haven't agreed on what call descriptor we want, this isn't super actionable, though. One nitpick: We get to decide whatever calling conventions these things have. We can even make that totally opaque, if we want to. So it we *want* them to be JavaScript function -like, we can. (With opaque, I mean: FastAccessorAssembler::GetParameter(size_t) exposes the calling convention. GetCallTarget() doesn't. We could delete GetParameter, and could offer GetCallTarget and the like.) https://codereview.chromium.org/1474543004/diff/80001/src/compiler/fast-acces... File src/compiler/fast-accessor-assembler.h (right): https://codereview.chromium.org/1474543004/diff/80001/src/compiler/fast-acces... src/compiler/fast-accessor-assembler.h:13: #include "src/compiler/raw-machine-assembler.h" On 2015/11/27 09:15:53, Michael Starzinger wrote: > I understand that RMA::Label still forces us to include this header, but as > discussed offline, we could avoid this by making "Label" not be an inner class. > Can we leave a TODO here in that regard? > > Also, since this header is allowed to be included from the outside, it should > contain the following comment (similar to pipeline.h) in front of the includes: > > // Clients of this interface shouldn't depend on lots of compiler internals. > // Do not include anything from src/compiler here! Done. (Comment. TODO now unnecessary, though.) https://codereview.chromium.org/1474543004/diff/80001/src/compiler/fast-acces... src/compiler/fast-accessor-assembler.h:13: #include "src/compiler/raw-machine-assembler.h" On 2015/11/27 09:26:23, Benedikt Meurer wrote: > How about making RMA::Label forward declarable first? Will be done, once cl/1477413002 goes through. https://codereview.chromium.org/1474543004/diff/80001/src/compiler/fast-acces... src/compiler/fast-accessor-assembler.h:32: // use by the API to implement Fast Dom Accessors. (crbug.com/508898). On 2015/11/27 09:15:53, Michael Starzinger wrote: > nit: Let's drop the reference to the "crbug" in the comment. Done. https://codereview.chromium.org/1474543004/diff/80001/src/compiler/fast-acces... src/compiler/fast-accessor-assembler.h:97: enum { BUILDING, BUILT, ERROR } state_; On 2015/11/27 09:26:23, Benedikt Meurer wrote: > Nit: Can we follow the style guide and use kBuilding, kBuilt and kError here? Done. We totally can. https://codereview.chromium.org/1474543004/diff/80001/test/cctest/test-api-fa... File test/cctest/test-api-fast-accessor-builder.cc (right): https://codereview.chromium.org/1474543004/diff/80001/test/cctest/test-api-fa... test/cctest/test-api-fast-accessor-builder.cc:55: ExpectInt32("f = new foo(); f.barf", 124); On 2015/11/27 14:32:54, Michael Starzinger wrote: > On 2015/11/27 10:21:17, mvstanton wrote: > > On 2015/11/27 09:15:53, Michael Starzinger wrote: > > > This test seems to assume that it takes exactly four runs to trigger the > > "fast" > > > accessor. I don't know the exact details behind this, but this looks > fragile. > > > Can this be made more robust? > > > > Good point. The code has the problem that it works accidentally. I read it and > > wondered how in the heck are we ratcheting up the IC state chain? It's because > > there is code/compilation caching going on for the same input strings. That's > a > > fragile assumption. I'd much prefer it to be written like: > > > > CompileRun("function bar(f) { return f.barf; }"); > > > > and then > > > > ExpectInt32("bar(new foo());", 123); // PREMONOMORPHIC > > ExpectInt32("bar(new foo());", 123); // MONOMORPHIC > > ExpectInt32("bar(new foo());", 124); // MONOMORPHIC handler called. > > > > That way it's clear that you are cycling the load through IC states. > > > > And the comment shows why you expect it to take until the third call to get > the > > right answer. > > > > Also...it really should be on the third call, not the 4th, right? I suspect > it's > > the 4th because of the implementation of the aforementioned code caching. > > SGTM. I am fine with wrapping it in a function and with the comments that > Michael suggested. Avoiding cycling through the IC states would probably be too > cumbersome. Changed this (& below) to wrap the accessor, and to provide a WARMUP macro that builds JS source to call the function N times. So there's still a secret parameter N, but it's now only in one place. It also has a comment explaining why it's there.
https://codereview.chromium.org/1474543004/diff/100001/src/compiler/fast-acce... File src/compiler/fast-accessor-assembler.cc (right): https://codereview.chromium.org/1474543004/diff/100001/src/compiler/fast-acce... src/compiler/fast-accessor-assembler.cc:147: Handle<Code> code = Pipeline::GenerateCodeForTesting( Is Pipeline::GenerateCodeForTesting the right call? Should I add a Pipeline::GenerateCodeForInterpreter-equivalent that, for the time being, just delegates to Pipeline::GenerateCodeForTesting?
https://codereview.chromium.org/1474543004/diff/80001/src/compiler/fast-acces... File src/compiler/fast-accessor-assembler.cc (right): https://codereview.chromium.org/1474543004/diff/80001/src/compiler/fast-acces... src/compiler/fast-accessor-assembler.cc:25: Linkage::GetJSCallDescriptor(&zone_, false, 1, On 2015/11/27 16:24:50, vogelheim wrote: > On 2015/11/27 10:04:56, epertoso wrote: > > On 2015/11/27 at 09:15:52, Michael Starzinger wrote: > > > Not sure if a JavaScript call descriptor is what we want here in the long > run. > > IIUC these accessors do not represent JavaScript functions and hence will not > > follow their calling convention. > > > > Currently we just tail-call these accessors, and in this way we don't have to > > move the arguments around. See http://crrev.com/1407313004. > > > > If you have a strong reason to use another call descriptor, I'm going to > modify > > the builtins and the handler compiler as well. > > Ack to both comments. As long as we haven't agreed on what call descriptor we > want, this isn't super actionable, though. > > One nitpick: We get to decide whatever calling conventions these things have. We > can even make that totally opaque, if we want to. So it we *want* them to be > JavaScript function -like, we can. > > (With opaque, I mean: FastAccessorAssembler::GetParameter(size_t) exposes the > calling convention. GetCallTarget() doesn't. We could delete GetParameter, and > could offer GetCallTarget and the like.) Acknowledged. Whatever allows you to make forward progress here is fine with me for now. It wasn't intended as an actionable comment, more of an "for posterity" comment. Just be prepared that the JavaScript call-descriptor might change as we move forward with TurboFan (e.g. we recently added Parameter(n) to be the new.target which moved arg-count to Parameter(n+1) and context to Parameter(n+2) for example). Similar such changes cannot be ruled out in the future. The JavaScript call-descriptor follows whatever the main pipeline deems necessary.
Still missing: - more tests. - C++ callback for FastAccessorBuilder. - test fail with --always-opt. Need to figure out w/ Enrico whether that's a failure in this CL, or an issue with Crankshaft-inline of 'fast accessors', or so. Fix will depend on that answer. https://codereview.chromium.org/1474543004/diff/80001/include/v8.h File include/v8.h (right): https://codereview.chromium.org/1474543004/diff/80001/include/v8.h#newcode6982 include/v8.h:6982: namespace experimental { On 2015/11/27 08:33:45, jochen wrote: > i wonder whether we should move this to a separate header? Done. In a separate CL, I'll also move the api-* files into a separate API directory. Do I need to check on v8-team first? https://codereview.chromium.org/1474543004/diff/80001/src/compiler/fast-acces... File src/compiler/fast-accessor-assembler.h (right): https://codereview.chromium.org/1474543004/diff/80001/src/compiler/fast-acces... src/compiler/fast-accessor-assembler.h:13: #include "src/compiler/raw-machine-assembler.h" On 2015/11/27 09:26:23, Benedikt Meurer wrote: > How about making RMA::Label forward declarable first? Done.
Please take another look, everyone. - Now with unit tests for all fast-accessor-builder ops. - Native callback still missing. I want to do that in a follow-up CL. - I'm opting out of --always-opt tests, for now.
https://codereview.chromium.org/1474543004/diff/160001/src/compiler/fast-acce... File src/compiler/fast-accessor-assembler.cc (right): https://codereview.chromium.org/1474543004/diff/160001/src/compiler/fast-acce... src/compiler/fast-accessor-assembler.cc:43: // For JS call descriptor, we can assume that the call target / 'this' pointer We need to use internal nomenclature here, otherwise all V8 devs will be highly confused. 1) "call target" -> Parameter(-1), refers to the closure (i.e. JSFunction object) that represents the target function of the call and that this code object has been attached to. 2) "receiver" (aka "this") -> Parameter(0), refers to the object acting as the message receiver for the method invocation. Implicitly that is the first parameter to the function, all other arguments have been shifted back by one. Actionable feedback: The comment should only talk about "receiver" or "this" and I would rename the function to GetReceiver() or something similar. https://codereview.chromium.org/1474543004/diff/160001/src/compiler/fast-acce... src/compiler/fast-accessor-assembler.cc:51: FastAccessorAssembler::ValueId FastAccessorAssembler::GetInternalField( nit: How would you feel about s/GetInternalField/LoadInternalField/ here? https://codereview.chromium.org/1474543004/diff/160001/src/compiler/fast-acce... src/compiler/fast-accessor-assembler.cc:56: assembler_->IntPtrConstant(JSObject::kHeaderSize - kHeapObjectTag + This calculation is only correct for JSObject, for several subclasses of JSObject which can also have internal fields the header size differs (i.e. see JSObject::GetHeaderSize). If we want to go with this restriction, then it needs to be clearly documented. https://codereview.chromium.org/1474543004/diff/160001/src/compiler/fast-acce... src/compiler/fast-accessor-assembler.cc:99: void FastAccessorAssembler::CheckNotNullOrReturnNull(ValueId value) { One "null" is more "null" than the other "null" ... s/CheckNotNullOrReturnNull/CheckNotZeroOrReturnNull/ https://codereview.chromium.org/1474543004/diff/160001/src/compiler/fast-acce... src/compiler/fast-accessor-assembler.cc:113: RawMachineLabel* label = new RawMachineLabel; Can we allocate the Labels in our Zone please? Otherwise they will leak if the Build() method below is never called. https://codereview.chromium.org/1474543004/diff/160001/src/compiler/fast-acce... src/compiler/fast-accessor-assembler.cc:124: void FastAccessorAssembler::CheckNotNullOrJump(ValueId value_id, Likewise s/CheckNotNullOrJump/CheckNotZeroOrJump/ https://codereview.chromium.org/1474543004/diff/160001/src/compiler/fast-acce... src/compiler/fast-accessor-assembler.cc:141: delete label; See comment above, please allocate in Zone and don't delete here. https://codereview.chromium.org/1474543004/diff/160001/src/compiler/fast-acce... src/compiler/fast-accessor-assembler.cc:148: MaybeHandle<Code> code = Pipeline::GenerateCodeForTesting( This definitely needs a big fat TODO that we should add a separate (non-testing) entry point into the Pipeline for this. https://codereview.chromium.org/1474543004/diff/160001/src/compiler/fast-acce... File src/compiler/fast-accessor-assembler.h (right): https://codereview.chromium.org/1474543004/diff/160001/src/compiler/fast-acce... src/compiler/fast-accessor-assembler.h:62: // .. call native. nit: Can we change these two comments into TODOs? Because I assume that's what they are intended for.
There's still bot failures, when --optimize-for-size is on. I still need to look into that. https://codereview.chromium.org/1474543004/diff/160001/src/compiler/fast-acce... File src/compiler/fast-accessor-assembler.cc (right): https://codereview.chromium.org/1474543004/diff/160001/src/compiler/fast-acce... src/compiler/fast-accessor-assembler.cc:43: // For JS call descriptor, we can assume that the call target / 'this' pointer On 2015/12/02 19:28:06, Michael Starzinger wrote: > We need to use internal nomenclature here, otherwise all V8 devs will be highly > confused. > > 1) "call target" -> Parameter(-1), refers to the closure (i.e. JSFunction > object) that represents the target function of the call and that this code > object has been attached to. > > 2) "receiver" (aka "this") -> Parameter(0), refers to the object acting as the > message receiver for the method invocation. Implicitly that is the first > parameter to the function, all other arguments have been shifted back by one. > > Actionable feedback: The comment should only talk about "receiver" or "this" and > I would rename the function to GetReceiver() or something similar. Done. https://codereview.chromium.org/1474543004/diff/160001/src/compiler/fast-acce... src/compiler/fast-accessor-assembler.cc:51: FastAccessorAssembler::ValueId FastAccessorAssembler::GetInternalField( On 2015/12/02 19:28:06, Michael Starzinger wrote: > nit: How would you feel about s/GetInternalField/LoadInternalField/ here? Done. https://codereview.chromium.org/1474543004/diff/160001/src/compiler/fast-acce... src/compiler/fast-accessor-assembler.cc:56: assembler_->IntPtrConstant(JSObject::kHeaderSize - kHeapObjectTag + On 2015/12/02 19:28:06, Michael Starzinger wrote: > This calculation is only correct for JSObject, for several subclasses of > JSObject which can also have internal fields the header size differs (i.e. see > JSObject::GetHeaderSize). If we want to go with this restriction, then it needs > to be clearly documented. Changed to check for JSObject, and to return Undefined if it doesn't match. I'm not sure that's the best way: Alternatively, the function could take a Label parameter so that the user can implement their own check-fail branch. https://codereview.chromium.org/1474543004/diff/160001/src/compiler/fast-acce... src/compiler/fast-accessor-assembler.cc:99: void FastAccessorAssembler::CheckNotNullOrReturnNull(ValueId value) { On 2015/12/02 19:28:06, Michael Starzinger wrote: > One "null" is more "null" than the other "null" ... > s/CheckNotNullOrReturnNull/CheckNotZeroOrReturnNull/ Done. https://codereview.chromium.org/1474543004/diff/160001/src/compiler/fast-acce... src/compiler/fast-accessor-assembler.cc:113: RawMachineLabel* label = new RawMachineLabel; On 2015/12/02 19:28:06, Michael Starzinger wrote: > Can we allocate the Labels in our Zone please? Otherwise they will leak if the > Build() method below is never called. Done. https://codereview.chromium.org/1474543004/diff/160001/src/compiler/fast-acce... src/compiler/fast-accessor-assembler.cc:124: void FastAccessorAssembler::CheckNotNullOrJump(ValueId value_id, On 2015/12/02 19:28:06, Michael Starzinger wrote: > Likewise s/CheckNotNullOrJump/CheckNotZeroOrJump/ Done. https://codereview.chromium.org/1474543004/diff/160001/src/compiler/fast-acce... src/compiler/fast-accessor-assembler.cc:141: delete label; On 2015/12/02 19:28:06, Michael Starzinger wrote: > See comment above, please allocate in Zone and don't delete here. Done. https://codereview.chromium.org/1474543004/diff/160001/src/compiler/fast-acce... src/compiler/fast-accessor-assembler.cc:148: MaybeHandle<Code> code = Pipeline::GenerateCodeForTesting( On 2015/12/02 19:28:06, Michael Starzinger wrote: > This definitely needs a big fat TODO that we should add a separate (non-testing) > entry point into the Pipeline for this. Done.
LGTM on "compiler" with some nits. Didn't look at the rest all too closely. https://codereview.chromium.org/1474543004/diff/180001/src/compiler/fast-acce... File src/compiler/fast-accessor-assembler.cc (right): https://codereview.chromium.org/1474543004/diff/180001/src/compiler/fast-acce... src/compiler/fast-accessor-assembler.cc:54: auto object_map = nit: The return type should be "Node*" here AFAICT. Can we write out "Node*" here and throughout the function here for readability? https://codereview.chromium.org/1474543004/diff/180001/src/compiler/fast-acce... src/compiler/fast-accessor-assembler.cc:205: FastAccessorAssembler::ValueId value) const { nit: Just "ValueId" without the namespace prefix should work for arguments IIRC. https://codereview.chromium.org/1474543004/diff/180001/src/compiler/fast-acce... src/compiler/fast-accessor-assembler.cc:213: FastAccessorAssembler::LabelId label) const { Likewise. https://codereview.chromium.org/1474543004/diff/180001/src/compiler/fast-acce... File src/compiler/fast-accessor-assembler.h (right): https://codereview.chromium.org/1474543004/diff/180001/src/compiler/fast-acce... src/compiler/fast-accessor-assembler.h:28: class CallDescriptor; nit: The "CallDescriptor" forward declaration seems to be unused.
https://codereview.chromium.org/1474543004/diff/180001/src/compiler/fast-acce... File src/compiler/fast-accessor-assembler.cc (right): https://codereview.chromium.org/1474543004/diff/180001/src/compiler/fast-acce... src/compiler/fast-accessor-assembler.cc:54: auto object_map = On 2015/12/03 13:36:18, Michael Starzinger wrote: > nit: The return type should be "Node*" here AFAICT. Can we write out "Node*" > here and throughout the function here for readability? Done. https://codereview.chromium.org/1474543004/diff/180001/src/compiler/fast-acce... src/compiler/fast-accessor-assembler.cc:205: FastAccessorAssembler::ValueId value) const { On 2015/12/03 13:36:18, Michael Starzinger wrote: > nit: Just "ValueId" without the namespace prefix should work for arguments IIRC. Done. https://codereview.chromium.org/1474543004/diff/180001/src/compiler/fast-acce... src/compiler/fast-accessor-assembler.cc:213: FastAccessorAssembler::LabelId label) const { On 2015/12/03 13:36:18, Michael Starzinger wrote: > Likewise. Done.
https://codereview.chromium.org/1474543004/diff/180001/src/compiler/fast-acce... File src/compiler/fast-accessor-assembler.h (right): https://codereview.chromium.org/1474543004/diff/180001/src/compiler/fast-acce... src/compiler/fast-accessor-assembler.h:28: class CallDescriptor; On 2015/12/03 13:36:18, Michael Starzinger wrote: > nit: The "CallDescriptor" forward declaration seems to be unused. Done.
lgtm
https://codereview.chromium.org/1474543004/diff/200001/include/v8-experimental.h File include/v8-experimental.h (right): https://codereview.chromium.org/1474543004/diff/200001/include/v8-experimenta... include/v8-experimental.h:49: void* impl_; Why don't you just forward-declare internal::compiler::FastAccessorAssembler? You won't need to reinterpret_cast anything that way. https://codereview.chromium.org/1474543004/diff/200001/include/v8.h File include/v8.h (right): https://codereview.chromium.org/1474543004/diff/200001/include/v8.h#newcode4423 include/v8.h:4423: static Local<FunctionTemplate> NewWithFastHandler( Why do we need two versions of this? The second one is not even implemented, btw. https://codereview.chromium.org/1474543004/diff/200001/src/api-experimental.cc File src/api-experimental.cc (right): https://codereview.chromium.org/1474543004/diff/200001/src/api-experimental.c... src/api-experimental.cc:22: v8::internal::MaybeHandle<v8::internal::Code> BuildCodeFromFastAccessorBuilder( You're already in v8::internal, there's no need to fully qualify it in this block. https://codereview.chromium.org/1474543004/diff/200001/src/compiler/fast-acce... File src/compiler/fast-accessor-assembler.cc (right): https://codereview.chromium.org/1474543004/diff/200001/src/compiler/fast-acce... src/compiler/fast-accessor-assembler.cc:95: FastAccessorAssembler::ValueId value, int offset) { Qualifying ValueId here and below is not necessary, when used as an argument. https://codereview.chromium.org/1474543004/diff/200001/src/compiler/fast-acce... File src/compiler/fast-accessor-assembler.h (right): https://codereview.chromium.org/1474543004/diff/200001/src/compiler/fast-acce... src/compiler/fast-accessor-assembler.h:87: base::SmartPointer<RawMachineAssembler> assembler_; Do you need this to be a pointer? https://codereview.chromium.org/1474543004/diff/200001/src/compiler/fast-acce... src/compiler/fast-accessor-assembler.h:89: // To prevent exposing the RMA internals to the outside world, we'll map This comment is outdated.
https://codereview.chromium.org/1474543004/diff/200001/include/v8-experimental.h File include/v8-experimental.h (right): https://codereview.chromium.org/1474543004/diff/200001/include/v8-experimenta... include/v8-experimental.h:49: void* impl_; On 2015/12/04 09:56:32, epertoso wrote: > Why don't you just forward-declare internal::compiler::FastAccessorAssembler? > You won't need to reinterpret_cast anything that way. Well... I'm trying to emulate the rest of v8.h, which consistently exposes a public data type, without any visible connection to the internal one and then reinterpret_cast<>s the public type to the internal one (and vice versa). Note how most API types don't have any data members at all. Not sure what to do here... Jochen/Enrico, is this API type isolation thing worthwhile? https://codereview.chromium.org/1474543004/diff/200001/include/v8.h File include/v8.h (right): https://codereview.chromium.org/1474543004/diff/200001/include/v8.h#newcode4423 include/v8.h:4423: static Local<FunctionTemplate> NewWithFastHandler( On 2015/12/04 09:56:32, epertoso wrote: > Why do we need two versions of this? The second one is not even implemented, > btw. Done. Yeah, that was an editing leftover. https://codereview.chromium.org/1474543004/diff/200001/src/api-experimental.cc File src/api-experimental.cc (right): https://codereview.chromium.org/1474543004/diff/200001/src/api-experimental.c... src/api-experimental.cc:22: v8::internal::MaybeHandle<v8::internal::Code> BuildCodeFromFastAccessorBuilder( On 2015/12/04 09:56:32, epertoso wrote: > You're already in v8::internal, there's no need to fully qualify it in this > block. Done. https://codereview.chromium.org/1474543004/diff/200001/src/compiler/fast-acce... File src/compiler/fast-accessor-assembler.cc (right): https://codereview.chromium.org/1474543004/diff/200001/src/compiler/fast-acce... src/compiler/fast-accessor-assembler.cc:95: FastAccessorAssembler::ValueId value, int offset) { On 2015/12/04 09:56:32, epertoso wrote: > Qualifying ValueId here and below is not necessary, when used as an argument. Done. https://codereview.chromium.org/1474543004/diff/200001/src/compiler/fast-acce... File src/compiler/fast-accessor-assembler.h (right): https://codereview.chromium.org/1474543004/diff/200001/src/compiler/fast-acce... src/compiler/fast-accessor-assembler.h:87: base::SmartPointer<RawMachineAssembler> assembler_; On 2015/12/04 09:56:32, epertoso wrote: > Do you need this to be a pointer? I do, if I don't want to include the src/compiler/raw-machine-assembler.h. The intention is to avoid re-exporting that header to all consumers of this one. https://codereview.chromium.org/1474543004/diff/200001/src/compiler/fast-acce... src/compiler/fast-accessor-assembler.h:89: // To prevent exposing the RMA internals to the outside world, we'll map On 2015/12/04 09:56:32, epertoso wrote: > This comment is outdated. Done.
https://codereview.chromium.org/1474543004/diff/200001/include/v8-experimental.h File include/v8-experimental.h (right): https://codereview.chromium.org/1474543004/diff/200001/include/v8-experimenta... include/v8-experimental.h:49: void* impl_; On 2015/12/08 at 12:52:56, vogelheim wrote: > On 2015/12/04 09:56:32, epertoso wrote: > > Why don't you just forward-declare internal::compiler::FastAccessorAssembler? > > You won't need to reinterpret_cast anything that way. > > Well... I'm trying to emulate the rest of v8.h, which consistently exposes a public data type, without any visible connection to the internal one and then reinterpret_cast<>s the public type to the internal one (and vice versa). Note how most API types don't have any data members at all. > > Not sure what to do here... Jochen/Enrico, is this API type isolation thing worthwhile? There are examples of both patterns, e.g. v8::DisallowJavascriptExecutionScope::internal_ vs. the namespace internal {} block at the top of v8.h I don't have a strong opinion for either
On 2015/12/08 at 13:00:42, jochen wrote: > https://codereview.chromium.org/1474543004/diff/200001/include/v8-experimental.h > File include/v8-experimental.h (right): > > https://codereview.chromium.org/1474543004/diff/200001/include/v8-experimenta... > include/v8-experimental.h:49: void* impl_; > On 2015/12/08 at 12:52:56, vogelheim wrote: > > On 2015/12/04 09:56:32, epertoso wrote: > > > Why don't you just forward-declare internal::compiler::FastAccessorAssembler? > > > You won't need to reinterpret_cast anything that way. > > > > Well... I'm trying to emulate the rest of v8.h, which consistently exposes a public data type, without any visible connection to the internal one and then reinterpret_cast<>s the public type to the internal one (and vice versa). Note how most API types don't have any data members at all. > > > > Not sure what to do here... Jochen/Enrico, is this API type isolation thing worthwhile? > > There are examples of both patterns, e.g. v8::DisallowJavascriptExecutionScope::internal_ vs. the namespace internal {} block at the top of v8.h > > I don't have a strong opinion for either Forward declared types are opaque anyway, it's just that the .cc will have slightly less boilerplate IMHO.
On 2015/12/08 13:12:49, epertoso wrote: > On 2015/12/08 at 13:00:42, jochen wrote: > > > https://codereview.chromium.org/1474543004/diff/200001/include/v8-experimental.h > > File include/v8-experimental.h (right): > > > > > https://codereview.chromium.org/1474543004/diff/200001/include/v8-experimenta... > > include/v8-experimental.h:49: void* impl_; > > On 2015/12/08 at 12:52:56, vogelheim wrote: > > > On 2015/12/04 09:56:32, epertoso wrote: > > > > Why don't you just forward-declare > internal::compiler::FastAccessorAssembler? > > > > You won't need to reinterpret_cast anything that way. > > > > > > Well... I'm trying to emulate the rest of v8.h, which consistently exposes a > public data type, without any visible connection to the internal one and then > reinterpret_cast<>s the public type to the internal one (and vice versa). Note > how most API types don't have any data members at all. > > > > > > Not sure what to do here... Jochen/Enrico, is this API type isolation thing > worthwhile? > > > > There are examples of both patterns, e.g. > v8::DisallowJavascriptExecutionScope::internal_ vs. the namespace internal {} > block at the top of v8.h > > > > I don't have a strong opinion for either > > Forward declared types are opaque anyway, it's just that the .cc will have > slightly less boilerplate IMHO. I've now changed FastAccessorBuilder to be a type without data members, like most other API types. I've also cleaned up the code to contain the reinterpret_cast<>s to to methods, FromApi and FromInternal. Hope this works well... (Assuming the tests pass on all bots, that's the version I'd like to submit.)
The CQ bit was checked by vogelheim@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mstarzinger@chromium.org, jochen@chromium.org Link to the patchset: https://codereview.chromium.org/1474543004/#ps260001 (title: "Remove impl_ cleanup. Drop tests for --optimize-for-size. Also rebase.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1474543004/260001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1474543004/260001
lgtm https://codereview.chromium.org/1474543004/diff/200001/src/compiler/fast-acce... File src/compiler/fast-accessor-assembler.h (right): https://codereview.chromium.org/1474543004/diff/200001/src/compiler/fast-acce... src/compiler/fast-accessor-assembler.h:87: base::SmartPointer<RawMachineAssembler> assembler_; On 2015/12/08 at 12:52:56, vogelheim wrote: > On 2015/12/04 09:56:32, epertoso wrote: > > Do you need this to be a pointer? > > I do, if I don't want to include the src/compiler/raw-machine-assembler.h. The intention is to avoid re-exporting that header to all consumers of this one. Sounds fair.
Message was sent while issue was closed.
Description was changed from ========== Implement FastAccessorBuilder. ... using the RawMachineAssembler and the work in cl/1407313004 BUG=chromium:508898 LOG=Y ========== to ========== Implement FastAccessorBuilder. ... using the RawMachineAssembler and the work in cl/1407313004 BUG=chromium:508898 LOG=Y ==========
Message was sent while issue was closed.
Committed patchset #14 (id:260001)
Message was sent while issue was closed.
Description was changed from ========== Implement FastAccessorBuilder. ... using the RawMachineAssembler and the work in cl/1407313004 BUG=chromium:508898 LOG=Y ========== to ========== Implement FastAccessorBuilder. ... using the RawMachineAssembler and the work in cl/1407313004 BUG=chromium:508898 LOG=Y Committed: https://crrev.com/515d9ccd8e6df7bf2ca01e2a55aaad30226399e1 Cr-Commit-Position: refs/heads/master@{#32742} ==========
Message was sent while issue was closed.
Patchset 14 (id:??) landed as https://crrev.com/515d9ccd8e6df7bf2ca01e2a55aaad30226399e1 Cr-Commit-Position: refs/heads/master@{#32742}
Message was sent while issue was closed.
A revert of this CL (patchset #14 id:260001) has been created in https://codereview.chromium.org/1513203002/ by vogelheim@chromium.org. The reason for reverting is: Broke the build, apparently..
Message was sent while issue was closed.
machenbach@chromium.org changed reviewers: + machenbach@chromium.org
Message was sent while issue was closed.
Another example for outdated tryjobs that the CQ reused... There was apparently some interaction with the patches that landed in between. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
