Two top level comments:
- The new additional phase seems expensive and bolted-on. I really think the
initialization of slots indices should be folded into the construction of
the
AST even though it conceptually could be considered separate.
- It seems that it would be possible to simplify how slots get allocated and
make the "client interface" to getting slots simpler (see comments below).
https://codereview.chromium.org/137403009/diff/690001/src/ast.cc
File src/ast.cc (right):
https://codereview.chromium.org/137403009/diff/690001/src/ast.cc#newcode728
src/ast.cc:728: if (CallFeedbackSlot() >= 0) {
You probably want to turn this into a convenience predicate
HasFeedbackSlot()
https://codereview.chromium.org/137403009/diff/690001/src/ast.h
File src/ast.h (right):
https://codereview.chromium.org/137403009/diff/690001/src/ast.h#newcode217
src/ast.h:217: virtual int ConsumeFeedbackSlots(Isolate* isolate, int
current_slot) {
Is it really consuming? Isn't it more like earmarking or reserving? A
comment might be useful here, too, e.g. "By default, AST nodes don't
store their type information in the type vector and therefore don't
reserve slots" or something to that effect.
https://codereview.chromium.org/137403009/diff/690001/src/ast.h#newcode1864
src/ast.h:1864: virtual int ConsumeFeedbackSlots(Isolate* isolate, int
current_slot) {
Somehow, this seems a little clunky. It seems like you want to have a
virtual method that returns the number of nodes needed by the AST node,
and that by default return zero, and only have to implement this method
if you want slots. For examples, if you change the consumefeedbackslots
logic in the ASTNode baseclass to call SetFirstFeedbackSlot and
GetFirstFeedbackSlot virtual methods that do nothing/ASSERT by default
and are only implemented for a new subclass of Expression called
ExpressionWithFeedback (which Call and ForIn derive from) you can get
the memory back and make adding slot support easy, just 1) subclass from
ExpressionWithFeedback and 2) Implement virtual int
GetFeedbackSlotCount().
https://codereview.chromium.org/137403009/diff/690001/src/compiler.cc
File src/compiler.cc (right):
https://codereview.chromium.org/137403009/diff/690001/src/compiler.cc#newcode64
src/compiler.cc:64: feedback_slots_(-1),
Don't you want to use kInvalidFeedbackSlot here and below?
https://codereview.chromium.org/137403009/diff/690001/src/feedbackslots.cc
File src/feedbackslots.cc (right):
https://codereview.chromium.org/137403009/diff/690001/src/feedbackslots.cc#newcode39
src/feedbackslots.cc:39: #define RECURSE(call)
\
Here and below, this seems like a lot of boilerplate to accomplish what
you want to do. Since we need the allocation slot information in common
cases, maybe it makes sense to fold it into building the AST? That
approach is probably less code and more efficient.
https://codereview.chromium.org/137403009/diff/690001/src/feedbackslots.h
File src/feedbackslots.h (right):
https://codereview.chromium.org/137403009/diff/690001/src/feedbackslots.h#newcode44
src/feedbackslots.h:44: explicit FeedbackSlotAllocator(Zone* zone) :
used_(0) {
I worry that the additional pass over the AST might be expensive,
especially for very short-running programs. E.g. did you measure
SunSpider performance?
https://codereview.chromium.org/137403009/diff/690001/src/ia32/code-stubs-ia32.cc
File src/ia32/code-stubs-ia32.cc (right):
https://codereview.chromium.org/137403009/diff/690001/src/ia32/code-stubs-ia32.cc#newcode2462
src/ia32/code-stubs-ia32.cc:2462: // ebx : Feedback vector
Boy, I wish we would generate all the code here and below in Hydrogen.
https://codereview.chromium.org/137403009/diff/690001/src/type-info.cc
File src/type-info.cc (right):
https://codereview.chromium.org/137403009/diff/690001/src/type-info.cc#newcode87
src/type-info.cc:87: !CanRetainOtherContext(JSFunction::cast(obj),
*native_context_))) {
nit: weird indentation
https://codereview.chromium.org/137403009/diff/690001/test/cctest/test-heap.cc
File test/cctest/test-heap.cc (right):
https://codereview.chromium.org/137403009/diff/690001/test/cctest/test-heap.cc#newcode2850
test/cctest/test-heap.cc:2850: // TODO(mvstanton): fix this test.
Yes, please do :-)
https://codereview.chromium.org/137403009/
--
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
---
You received this message because you are subscribed to the Google Groups "v8-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
For more options, visit https://groups.google.com/groups/opt_out.
