Hi guys, addressed comments, PTAL. 3 notes:
1) You guys had different thoughts on the way feedback slots should be
allocated, Benedikt favoring an Ast graph traversal, and Danno favoring a
work
list built up during parse, and later traversed after scope analysis. I've
now
gone with Danno's approach. Danno, I also addressed your feedback to avoid
adding a virtual function to AstNode with the use of multiple inheritance
and
template magic (including import of a file from the LOKI library).
2) It was hard to pin down a chokepoint where the processing of the deferred
list should be done. A weakness here is the addition of a "magic" parameter
to
BuildFunctionInfo(). I'll work to understand that better.
3) Golem heap/space size influenced the decision in
DeferredFeedbackSlotProcessor to go ahead and process nodes right away if
they
know the number of slots they want at parse time. We get positive results
if we
do that, negative otherwise. The difference is (needlessly) enqueuing all
CallNew and ForInStatement AstNodes for the sake of uniform processing.
Happily,
this decision is contained entirely within DeferredFeedbackSlotProcessor,
and
hopefully doesn't mar the design beyond existing accomodations.
Thanks!
--Michael
https://codereview.chromium.org/137403009/diff/480001/src/ast.cc
File src/ast.cc (right):
https://codereview.chromium.org/137403009/diff/480001/src/ast.cc#newcode727
src/ast.cc:727: if (CallFeedbackSlot() >= 0) {
On 2014/01/24 11:29:54, Benedikt Meurer wrote:
!= kInvalidFeedbackSlot
Done.
https://codereview.chromium.org/137403009/diff/480001/src/ast.h
File src/ast.h (right):
https://codereview.chromium.org/137403009/diff/480001/src/ast.h#newcode197
src/ast.h:197: maximum_feedback_slots_ += maximum;
On 2014/01/24 11:29:54, Benedikt Meurer wrote:
Can we have an ASSERT here to make sure that minimum_feedback_slots_
<=
maximum_feedback_slots_?
Changed the approach totally...
https://codereview.chromium.org/137403009/diff/480001/src/full-codegen.cc
File src/full-codegen.cc (right):
https://codereview.chromium.org/137403009/diff/480001/src/full-codegen.cc#newcode395
src/full-codegen.cc:395: feedback_vector_ =
isolate->factory()->NewFixedArrayWithHoles(minimum_length,
On 2014/01/24 11:29:54, Benedikt Meurer wrote:
As discussed offline: Please add an explanation here, that we have to
do this
for now because of our parser.
Changed to a new approach.
https://codereview.chromium.org/137403009/diff/480001/src/ia32/code-stubs-ia32.cc
File src/ia32/code-stubs-ia32.cc (right):
https://codereview.chromium.org/137403009/diff/480001/src/ia32/code-stubs-ia32.cc#newcode5814
src/ia32/code-stubs-ia32.cc:5814:
On 2014/01/24 11:29:54, Benedikt Meurer wrote:
Add an AssertIsSmi(edx).
Done.
https://codereview.chromium.org/137403009/diff/480001/src/ia32/code-stubs-ia32.cc#newcode5821
src/ia32/code-stubs-ia32.cc:5821: // TODO(mvstanton): change this
message below.
On 2014/01/24 11:29:54, Benedikt Meurer wrote:
Please fix this TODO.
Done.
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) {
On 2014/01/28 08:27:17, danno wrote:
You probably want to turn this into a convenience predicate
HasFeedbackSlot()
Done.
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) {
On 2014/01/28 08:27:17, danno wrote:
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.
Changed the approach quite a bit.
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)
\
On 2014/01/28 08:27:17, danno wrote:
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.
Done.
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) {
On 2014/01/28 08:27:17, danno wrote:
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?
Although it didn't appear to cost much (nodes in the cache, most likely,
because it's done after the scope analysis pass), it may be that walking
over the AST is too much. So I followed your advice elsewhere and
created a list of deferred nodes to process later.
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_))) {
On 2014/01/28 08:27:17, danno wrote:
nit: weird indentation
Addressed, but have another look.
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.
On 2014/01/28 08:27:17, danno wrote:
Yes, please do :-)
Done.
https://codereview.chromium.org/137403009/diff/750001/src/ast.h
File src/ast.h (right):
https://codereview.chromium.org/137403009/diff/750001/src/ast.h#newcode2970
src/ast.h:2970: : dont_optimize_reason_(kNoReason),
pass the deferred worker here.
https://codereview.chromium.org/137403009/diff/750001/src/compiler.cc
File src/compiler.cc (right):
https://codereview.chromium.org/137403009/diff/750001/src/compiler.cc#newcode248
src/compiler.cc:248: if (function()->slot_count() >= 0) {
turn this to an assedrt.
https://codereview.chromium.org/137403009/diff/750001/src/compiler.h
File src/compiler.h (right):
https://codereview.chromium.org/137403009/diff/750001/src/compiler.h#newcode184
src/compiler.h:184: ProcessFeedbackSlots();
pull this out.
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.
