[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
http://bugs.webkit.org/show_bug.cgi?id=16202
Attachment 18293: Revised proposed patch
http://bugs.webkit.org/attachment.cgi?id=18293&action=edit
------- 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
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.
More information about the webkit-reviews
mailing list