Hi Benedikt, hi Danno,
I've responded per our several conversations. Thanks for the help!
--Michael
https://codereview.chromium.org/137403009/diff/820001/src/arm/code-stubs-arm.cc
File src/arm/code-stubs-arm.cc (right):
https://codereview.chromium.org/137403009/diff/820001/src/arm/code-stubs-arm.cc#newcode3020
src/arm/code-stubs-arm.cc:3020: // Cache the called function in a global
property cell. Cache states
On 2014/02/04 08:53:50, Benedikt Meurer wrote:
Nit: global property cell -> feedback vector slot.
Done.
https://codereview.chromium.org/137403009/diff/820001/src/arm/code-stubs-arm.cc#newcode3080
src/arm/code-stubs-arm.cc:3080: // Create an AllocationSite if we don't
already have it, store it in the cell
On 2014/02/04 08:53:50, Benedikt Meurer wrote:
Nit: cell -> slot.
Done.
https://codereview.chromium.org/137403009/diff/820001/src/ast.h
File src/ast.h (right):
https://codereview.chromium.org/137403009/diff/820001/src/ast.h#newcode918
src/ast.h:918: class ForInStatement V8_FINAL : public ForEachStatement,
On 2014/02/04 08:53:50, Benedikt Meurer wrote:
You mentioned offline that you got rid of this multiple inheritance.
Why is it
still around?
Per our conversation, I'll keep the interface inheritance instead of
adding virtual functions to AstNode. One benefit of this approach is
that we won't end up wondering later if our new AstNode type has to
implement those functions.
https://codereview.chromium.org/137403009/diff/820001/src/compiler.cc
File src/compiler.cc (right):
https://codereview.chromium.org/137403009/diff/820001/src/compiler.cc#newcode246
src/compiler.cc:246: void CompilationInfo::ProcessFeedbackSlots() {
On 2014/02/04 08:53:50, Benedikt Meurer wrote:
Why is this in CompilationInfo? And why do we need three different
call sites
for this in the pipeline?
I managed to reduce this to a single chokepoint, SetScope(), now renamed
InitializeScope().
https://codereview.chromium.org/137403009/diff/820001/src/compiler.cc#newcode247
src/compiler.cc:247: // Scope analysis must have been done.
On 2014/02/04 08:53:50, Benedikt Meurer wrote:
Can we ASSERT that?
It's enough to do the work in InitializeScope() now, after the scope has
been set. I couldn't find a good handle on Scope to make sure it's
complete. There is a predicate, already_resolved(), but it doesn't
appear to do exactly what I need (private to Scope).
https://codereview.chromium.org/137403009/diff/820001/src/compiler.h
File src/compiler.h (right):
https://codereview.chromium.org/137403009/diff/820001/src/compiler.h#newcode641
src/compiler.h:641: bool process_feedback = true);
On 2014/02/04 08:53:50, Benedikt Meurer wrote:
This bool parameter is really confusing. If we really need it here to
make
things work, we should consider fixing our pipeline first, i.e. split
this
method into two methods.
Took some effort but I finally figured out another way.
The real chokepoint for this work is
CompilationInfo::SetScope(), because when the scope is applied, the
deferred work can be performed. However because this is a pile of work
that has to be done, Danno noticed this was bad because "SetScope"
implies a simple setter rather than a method that does work. So I
renamed the method to InitializeScope(), indicating with a comment that
some deferred work may be performed.
In this way I don't need to even mention feedback slots in the public
interface of CompilationInfo.
https://codereview.chromium.org/137403009/diff/820001/src/feedback_slots.h
File src/feedback_slots.h (right):
https://codereview.chromium.org/137403009/diff/820001/src/feedback_slots.h#newcode38
src/feedback_slots.h:38: class FeedbackSlotInterface {
On 2014/02/04 08:53:50, Benedikt Meurer wrote:
Hm, do we really need a new header file for this interface?
Since deferred slot processor is rather involved, ast.h is already 3400
lines, and feedback slots are a feature used by a distinct subset of
nodes only, it seems like good style to keep this activity out of the
main path.
https://codereview.chromium.org/137403009/diff/820001/src/type-info.cc
File src/type-info.cc (right):
https://codereview.chromium.org/137403009/diff/820001/src/type-info.cc#newcode84
src/type-info.cc:84: if (obj->IsSmi() || obj->IsAllocationSite() ||
On 2014/02/04 08:53:50, Benedikt Meurer wrote:
Please remove this left over hacks, and use something less obscure,
i.e.:
if (!obj->IsJSFunction() ||
!CanRetainOtherContext(JSFunction::cast(obj),
*native_context_)))
Extra points if you generalize CanRetainOtherContext() to take an
Object* and do
the IsJSFunction() check itself.
Thx, I removed the smi and allocationsite check. I reformulated the rest
as you described, but I didn't generalize for the reason that there is
also a CanRetainOtherContext(Map*, ...) method and if I provide a method
that takes Object* then the natural implication is that it should be
able to call the Map* as well. So it'll still be a little bit wordy, but
this way CanRetainOtherContext() remains very sharply defined as being
able to operate on a Map or a JSFunction.
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.
