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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Sun Jan 6 11:54:24 PST 2008

Darin Adler <darin at apple.com> has denied Cameron Zwarich (cpst)
<cwzwarich at uwaterloo.ca>'s request for review:
Bug 16202: Optimize allocation of ActivationImp objects

Attachment 18293: Revised proposed patch

------- Additional Comments from Darin Adler <darin at apple.com>
I think this looks really good. Lets get it landed!

Despite your reason for not including it, I still wish there was a change log

I suggest naming the new header file Activation.h -- we're going to rename
ActivationImp to Activation soon, and it would be nice not to have to rename
the file.

Why all the additional includes? Without a change log, I don't understand the
need for them and for changes like the one where you add the KJS:: prefix in


As per style guidelines, this should be ActivationImp_h, not


Something like this does not need to be a define. Instead, it should be a

    const int activationStackSize = 12;

(Or maybe unsigned or size_t.)

 36	class StackActivation;
 37	class FunctionImp;
 38	class Arguments;

These should be sorted alphabetically.

 41	    friend class StackActivation;
 42	    friend class JSGlobalObject;

Do we really need these friend declarations? We usually try to avoid them if

 44	    using JSVariableObject::JSVariableObjectData;

How does this "using" help us?

 46	    struct ActivationImpData : public
JSVariableObject::JSVariableObjectData {

Because of the "using" you don't need to qualify the JSVariableObjectData here
with the class name.

 48		ActivationImpData(const ActivationImpData&);

It's a minor problem that you have a copy constructor here without an
assignment operator. Perhaps you should inherit from Noncopyable to make sure
you get a compiler error if you try to do assignment on this. StackActivation
has the same issue.

Why not call it "ActivationData" instead of "ActivationImpData"?

 61	    virtual ~ActivationImp()
 62	    {
 63		if (!d()->onStack)
 64		    delete d();
 65	    }

This should be in the .cpp file, not the header.

 80	    bool isOnStack() { return d()->onStack; }

Maybe this should be a const member?

 86	    ActivationImpData* d() { return
static_cast<ActivationImpData*>(JSVariableObject::d); }

I am not a fan of the name "d()" for this. I'd prefer a word rather than a

 93	    ~StackActivation() { } 

This destructor definition has no effect. You should omit it.

108	 ActivationImp* activation = new ActivationImp(this);
109	 m_activation = activation;
110	 m_localStorage = &activation->localStorage();
111	 m_variableObject = activation;
112	 m_scopeChain.push(activation);

 110	 m_activation = globalObject->pushActivation(this);
 111	 m_localStorage = &m_activation->localStorage();
 112	 m_variableObject = m_activation;
 113	 m_scopeChain.push(m_activation);

Why did you eliminate the use of a local variable here? Seems unrelated to the
change, makes the patch bigger, and might even make things less efficient. Or
does it make things more efficient?

 119	 if (m_codeType == FunctionCode &&
 120	     m_globalObject->popActivation();

Seeing this code makes me think we should have a derived class for each of the
3 kinds of ExecState, so we don't have to have the check for m_codeType ==
FunctionCode in the destructor. Or we could just have the one additional kind,

 80	    void setScopeChainTop(JSObject* o) { m_scopeChain.set(o); }

I think the word "replace" might be clearer here than "set".

3232 #include "SavedBuiltins.h"
 33 #include "ActivationImp.h"
3334 #include "array_object.h"

We normally keep these lists of includes sorted alphabetically.

 525	 d()->activationCount--;

Since you use post-increment when pushing, I think pre-decrement here when
popping would be good instead of a separate line of code for the decrement.

 532	     if (!d()->activationCount) {
 533		 ActivationStackNode* prev = d()->activations->prev;
 534		 delete d()->activations;
 535		 d()->activations = prev;
 536		 d()->activationCount = KJS_ACTIVATION_STACK_SIZE;
 537	     }
 539	     ActivationImp* newActivation = new
ActivationImp(&d()->activations->data[d()->activationCount - 1]);
 541	     popActivation();

This looks inefficient, since popActivation is going to check activationCount
against 0 again. Is there a way to write this to avoid the double work? Is
there a way to write it to avoid the copied and pasted code to delete the

 235	     ActivationImp* pushActivation(ExecState* exec);
 236	     void popActivation();
 237	     void tearOffActivation(ExecState* exec);

We normally omit names of parameters like this "exec" here.

 59		JSVariableObjectData() { }

 81	    JSVariableObject() { }

How are these empty constructors helpful?

= 66		 JSVariableObjectData(const JSVariableObjectData& old)
 67		{
 68		    localStorage.reserveCapacity(old.localStorage.size());
 70		    for (LocalStorage::const_iterator iter =
old.localStorage.begin(); iter != old.localStorage.end(); ++iter)
 71			localStorage.append(*iter);
 73		    symbolTable = old.symbolTable;
 74		}

This looks identical to what the compiler-generated copy constructor would do.
And I expect it would do it more efficiently too. Can you just omit this?

 91	  e->dynamicGlobalObject()->tearOffActivation(e);
9092	   return e->activationObject()->get(exec, propertyName);

I suggest that we change the idiom here so that there's a single function that
gets you the activation object, tearing it off if needed. The callers should
make the call directly on ExecState and not have to talk directly to the global

 347 ActivationImp::ActivationImp(StackActivation* oldStackEntry)
 348 {
 349   JSVariableObject::d = new
 350 }

This should be indented 4 to match the rest of the file.

 388	     ExecState* e = exec;
 389	     while (e) {
 390		 if (e->function() == d()->function) {
 391		     e->dynamicGlobalObject()->tearOffActivation(e);
 392		     ActivationImp* newActivation = e->activationObject();
 393		     slot.setCustom(newActivation,
 394		     return true;
 395		 }
 397		 e = e->callingExecState();
 398	     }

This should be written as a for loop so it's easier to see the loop structure.

 436	 size_t size = d()->localStorage.size();
 438	 for (size_t i = 0; i < size; ++i) {
 439	     JSValue* value = d()->localStorage[i].value;
 441	     if (!value->marked())
 442		 value->mark();
 443	 }

It seems that we want to hoist d()->localStorage out of the loop, unless we
think the compiler will do it for us.

 460 ActivationImp::ActivationImpData::ActivationImpData(const
ActivationImpData& old)
 461   : JSVariableObjectData(old)
 462 {
 463	 exec = old.exec;
 464	 function = old.function;
 465	 argumentsObject = old.argumentsObject;
 466	 onStack = false;
 467 }

Why not use construction syntax here instead of assignment syntax?

12861286     base = *iter;
12871287     if (base->getPropertySlot(exec, m_ident, slot)) {
 1288	      base = *iter;

Why this change? This really needs a comment, in the change log if nowhere

 2  *  This file is part of the KDE libraries

Please leave that out of new source files.

 32		if ((o->isActivationObject() &&
static_cast<ActivationImp*>(o)->isOnStack()) || !o->marked())
 33		    o->mark();

I think this needs a comment. I also think that the function to mark activation
objects that are on the stack should be something other than "mark()" itself.
That would save a branch inside the function because the new function would
only be for the activations on the stack and the override of mark would only be
for the activations that are not.

I'm tempted to just make all these revisions myself and land this. But I'm busy
with something else at the moment -- I'll mark this review-, but if I have time
I'll grab this and work on whipping into a shape a little and landing it later.
If I do that I'll add a comment and assign this bug to myself to indicate I'm
doing so.

More information about the webkit-reviews mailing list