[Webkit-unassigned] [Bug 16202] Optimize allocation of ActivationImp objects

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


http://bugs.webkit.org/show_bug.cgi?id=16202


darin at apple.com changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #18293|review?                     |review-
               Flag|                            |




------- Comment #15 from darin at apple.com  2008-01-06 11:54 PDT -------
(From update of attachment 18293)
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
here.

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
NP_jsobject.cpp.

 25 #ifndef KJS_ACTIVATIONIMP_H
 26 #define KJS_ACTIVATIONIMP_H

As per style guidelines, this should be ActivationImp_h, not
KJS_ACTIVATIONIMP_H.

 32 #define KJS_ACTIVATION_STACK_SIZE 32

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

    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
possible.

 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
letter.

 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 &&
static_cast<ActivationImp*>(m_activation)->isOnStack())
 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,
FunctionExecState.

 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--;
 526    
d()->activations->data[d()->activationCount].activationDataStorage.localStorage.shrink(0);

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         }
 538         
 539         ActivationImp* newActivation = new
ActivationImp(&d()->activations->data[d()->activationCount - 1]);
 540         
 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
activation?

 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());
 69                 
 70                 for (LocalStorage::const_iterator iter =
old.localStorage.begin(); iter != old.localStorage.end(); ++iter)
 71                     localStorage.append(*iter);
 72                 
 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
object.

 347 ActivationImp::ActivationImp(StackActivation* oldStackEntry)
 348 {
 349   JSVariableObject::d = new
ActivationImpData(oldStackEntry->activationDataStorage);
 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,
newActivation->getArgumentsGetter());
 394                 return true;
 395             }
 396 
 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();
 437     
 438     for (size_t i = 0; i < size; ++i) {
 439         JSValue* value = d()->localStorage[i].value;
 440         
 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
else.

 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.


-- 
Configure bugmail: http://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.



More information about the webkit-unassigned mailing list