[webkit-reviews] review granted: [Bug 16202] Optimize allocation of ActivationImp objects : [Attachment 18344] Revised proposed patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Cameron Zwarich (cpst)
<cwzwarich at uwaterloo.ca>'s request for review:
Bug 16202: Optimize allocation of ActivationImp objects
http://bugs.webkit.org/show_bug.cgi?id=16202

Attachment 18344: Revised proposed patch
http://bugs.webkit.org/attachment.cgi?id=18344&action=edit

------- Additional Comments from Darin Adler <darin at apple.com>
This looks great. I am going to say review+, although if you decide to review
this you may want to clear the review flag.

 27 #include "JSVariableObject.h"
 28 #include "object.h"
 29 #include "ExecState.h"

Should sort includes alphabetically. (We normally do this with straight "sort",
which puts the capital letters before the lowercase.)

 41	    struct ActivationData : public
JSVariableObject::JSVariableObjectData {

I think the fact that JSVariableObject is a base class means you won't have to
qualify this at all. You could try removing the JSVariableObject prefix.

 59	    void init(ExecState* exec);

We normally omit the argument name in cases like this where it's obvious.

 74	    bool needsPop() const { return (d()->isOnStack || d()->leftRelic);
}

I don't think the parentheses are helpful here.

 78	    static JSValue* argumentsGetter(ExecState*, JSObject*, const
Identifier&, const PropertySlot& slot);

Same rule here about omitting argument names -- we'd normally omit that word
"slot".

 83	const size_t activationStackSize = 32;

This isn't actually a stack size. It's the size of the chunk of stack stored in
each node, so it ought to have a name that doesn't make it seem like it's the
size of the entire activation stack.

2828 #include "JSGlobalObject.h"
 29 #include "Activation.h"
2930 #include "function.h"
3031 #include "internal.h"
 32 #include "scope_chain_mark.h"

Activation.h should go first alphabetically, before JSGlobalObject.h.

 120	 if (m_codeType == FunctionCode &&
static_cast<ActivationImp*>(m_activation)->needsPop())
 121	     m_globalObject->popActivation();

Why do you need to cast m_activation to ActivationImp*? Isn't that already the
type of m_activation?

 504	 if (d()->activationCount == activationStackSize) {
 505	     ActivationStackNode* newNode = new ActivationStackNode;
 506	     newNode->prev = d()->activations;
 507	     d()->activations = newNode;
 508	     d()->activationCount = 0;
 509	   }

Closing brace here is indented incorrectly.

 1286	      // If m_ident is 'arguments', the getPropertySlot() call may
cause 
 1287	      // base (which must be an ActivationImp in such this case) to be
torn
 1288	      // off from the activation stack, in which case we need to get it
again
 1289	      // from the ScopeChainIterator.
 1290 
 1291	      JSObject* base = *iter;

Good comment, but it's less important now that there is no local variable base
set up before getPropertySlot. And I feel bad for asking you to put it in -- we
don't want the same comment copied and pasted at every call site. Should
probably just have a comment that points at the top one (see xxx, where xxx is
the name of the first function).

 30		JSObject *o = n->object;

The * should go next to the type, JSObject*.

 33		// JSobject::mark() method called on it, because it doesn't
have an

Should say JSObject with a capital O. Also, it's not just JSObject::mark() that
can't be called on it, but more importantly JSObject::marked().

 83	    void replace(JSObject*);

I think this should be called replaceTop, not just replace.