RESOLVED FIXED 16202
Optimize allocation of ActivationImp objects
https://bugs.webkit.org/show_bug.cgi?id=16202
Summary Optimize allocation of ActivationImp objects
Cameron Zwarich (cpst)
Reported 2007-11-30 03:58:15 PST
During the recent flood of optimization of JavaScriptCore, profiling has revealed that a major issue for performance in the existing interpreter is the allocation of ActivationImp objects on the JS heap. In particular, this is likely the cause for JSC's poor performance on control-flow heavy benchmarks in the SunSpider suite relative to other benchmarks. Most ActivationImp objects do not need to be allocated in the garbage collected heap, because they only exist for a static extent. We would like to keep a WTF::Vector of ActivationImps that we can use as a stack for most purposes. The question is when we need an ActivationImp to be torn off and be heap allocated. For now, I think we need to do it in the following cases: 1) eval 2) with 3) try ... catch 4) local function definitions 5) function expressions If I am missing anything, please let me know. Some of these may be possible to eliminate with some form of eval limiting (see bug 15759), but that is a hard problem, so we can't rely on it being solved for now. We may also be able to determine a number of these situations statically and mark the node tree so we don't even allocate on the ActivationImp stack in the first place. Some issues creep up in the implementation. Every ActivationImp also heap allocates a private data structure and keeps a pointer to it (to get around the JSObject size restriction), so we need to also allocate this information on the stack. My idea was to create a friend class StackActivation that stores both an ActivationImp and an ActivationImpPrivate object: We need to be sure we update all the pointers to the ActivationImp or ActivationImpPrivate when we tear off onto the heap. As far as I can tell, the only ones that need to be updated are: 1) The pointer from the ActivationImp to the ActivationImpPrivate 2) ExecState::m_activation 3) ExecState::m_variable 4) The top of the scope chain 5) ExecState::m_localStorageBuffer (points into the local storage allocated in ActivationImpPrivate) Is this all of them? I should note that a proper implementation of this optimization is particularly important for any future bytecode interpreter, because the gains that come from not allocating activation frames on the heap will be more pronounced in that situation.
Attachments
Patch in progress (10.06 KB, patch)
2007-12-02 05:04 PST, Cameron Zwarich (cpst)
no flags
Revised patch in progress (11.84 KB, patch)
2007-12-21 00:25 PST, Cameron Zwarich (cpst)
no flags
Revised patch in progress (12.56 KB, patch)
2007-12-26 23:11 PST, Cameron Zwarich (cpst)
no flags
Revised patch in progress (13.05 KB, patch)
2008-01-01 23:24 PST, Cameron Zwarich (cpst)
no flags
Revised patch in progress (15.85 KB, patch)
2008-01-02 03:16 PST, Cameron Zwarich (cpst)
no flags
Proposed patch (26.12 KB, patch)
2008-01-05 04:45 PST, Cameron Zwarich (cpst)
no flags
Revised proposed patch (26.12 KB, patch)
2008-01-05 18:26 PST, Cameron Zwarich (cpst)
no flags
Revised proposed patch (26.22 KB, patch)
2008-01-05 19:15 PST, Cameron Zwarich (cpst)
darin: review-
Revised proposed patch (31.63 KB, patch)
2008-01-06 18:44 PST, Cameron Zwarich (cpst)
mjs: review-
Revised proposed patch (23.55 KB, patch)
2008-01-08 07:09 PST, Cameron Zwarich (cpst)
darin: review-
Revised proposed patch (34.60 KB, patch)
2008-01-08 22:04 PST, Cameron Zwarich (cpst)
zwarich: review-
Revised proposed patch (33.32 KB, patch)
2008-01-09 00:51 PST, Cameron Zwarich (cpst)
mjs: review+
Eric Seidel (no email)
Comment 1 2007-11-30 04:21:01 PST
Hum... so now all we need is the patch. :)
Maciej Stachowiak
Comment 2 2007-11-30 05:52:21 PST
The analysis here seems generally sound. I said on IRC that I'm not sure you need to tear off in cases 2 and 3. But you may want to anyway, since then tearoff is always for the current top of the scope chain, even though sometimes it happens before the moment a closure truly needs it.
Geoffrey Garen
Comment 3 2007-11-30 11:30:37 PST
In the general case, where we don't need an ActivationImp, I think we can just put LocalStorage directly into the stack-allocated ExecState. When we do need an ActivationImp, we can allocate it, and move LocalStorage from the ExecState to the ActivationImp.
Cameron Zwarich (cpst)
Comment 4 2007-11-30 15:13:21 PST
(In reply to comment #2) > The analysis here seems generally sound. I said on IRC that I'm not sure you > need to tear off in cases 2 and 3. But you may want to anyway, since then > tearoff is always for the current top of the scope chain, even though sometimes > it happens before the moment a closure truly needs it. Cases 2 and 3 push an object on the top of the scope chain, so that when we would go to tear off an ActivationImp upon seeing a function expression, we would have to change pointers to the ActivationImp further up the scope chain. Am I incorrect here? (In reply to comment #3) > In the general case, where we don't need an ActivationImp, I think we can just > put LocalStorage directly into the stack-allocated ExecState. When we do need > an ActivationImp, we can allocate it, and move LocalStorage from the ExecState > to the ActivationImp. Doesn't JavaScriptCore already have stack space issues? Is there a consensus that this is the best idea?
Maciej Stachowiak
Comment 5 2007-12-01 02:33:59 PST
When Geoff says "stack-allocated" he does not necessarily mean the machine stack, more likely he means an explicit stack managed as a Vector or the like.
Cameron Zwarich (cpst)
Comment 6 2007-12-02 05:04:21 PST
Created attachment 17642 [details] Patch in progress Here's an implementation of the basic idea. I used a Deque instead of a Vector, because on resize a Vector will copy all of the deeper ActivationImps as well, and those will have pointers into them from ExecStates further up the stack. For this to actually be a performance gain we will have to implement a segmented stack, but that will be easy once we get the mechanics right. I didn't actually insert the calls to Interpreter::tearOffActivation() everywhere, because I want to check a few more things. However, I uploaded it now since I have a few other things to do today and Eric has been on my case to upload something for a while. ;-) No ChangeLog because it's clearly not meant for review.
Cameron Zwarich (cpst)
Comment 7 2007-12-21 00:25:21 PST
Created attachment 18026 [details] Revised patch in progress Here's a new version of the patch. It runs most tests in the JSC test suite, but it still segfaults on the odd one, which perhaps means that there is a reference to the old ActivationImp lying around that I missed. I also frequently get the error: ERROR: free is not supported (/Users/Cameron/WebKit/JavaScriptCore/kjs/CollectorHeapIntrospector.h:56 static void KJS::CollectorHeapIntrospector::zoneFree(malloc_zone_t*, void*)) However, this is probably due to the destructor automatically being called on the ActivationImp that is a member of StackActivation. I think this patch is pretty close to being completely functional. When it is, we'll just need to make the right segmented stack data structure so we can get some performance out of it.
Cameron Zwarich (cpst)
Comment 8 2007-12-26 23:11:07 PST
Created attachment 18125 [details] Revised patch in progress Here is a revised version of my patch in progress. I am focusing on getting it to pass all of the JSC tests without crashing. It is not quite there, but there are only a handful left. Running testkjs with debug output on some of the examples (e.g. /ecma/Date/15.9.5.7.js) reports that there are a number of KJS::Node leaks. I don't yet know if these are spurious, but I hope so.
Cameron Zwarich (cpst)
Comment 9 2008-01-01 23:24:50 PST
Created attachment 18234 [details] Revised patch in progress Just another update of the patch in progress. I am in the middle of fixing up the interaction with the GC, as that might be the cause of the few remaining problems. After I do some reorganization of header files for dependencies I'll upload another updated version.
Cameron Zwarich (cpst)
Comment 10 2008-01-02 03:16:44 PST
Created attachment 18236 [details] Revised patch in progress Well, this version passes all of the JSC tests in both Debug and Release modes without any problems. I also eliminated the cause of the CollectorHeapIntrospector warnings. I'll merge it with a newer tree tomorrow, add the correct forwarding headers, and run the JS layout tests. I'm not expecting any issues. If that works then the only remaining issue is the inefficiency WTF::Deque data structure. Also, is there a better way to copy the ActivationImpPrivate::localStorage than I did in this code?
Cameron Zwarich (cpst)
Comment 11 2008-01-05 04:45:29 PST
Created attachment 18287 [details] Proposed patch Here it is, passing all tests! I moved from a WTF::Deque data structure to an explicit list of arrays of activation records, which exposed some bugs that were not present before, most likely due to memory for each of the Deque records not being reused by anything else after being freed. I think it's pretty solid at this point. The JSC and layout tests contain a lot of the sort of things that might trip up a patch like this. Here are the Sunspider results: TEST COMPARISON FROM TO DETAILS ============================================================================= ** TOTAL **: 1.044x as fast 3321.7ms +/- 0.2% 3180.5ms +/- 0.1% significant ============================================================================= 3d: 1.014x as fast 401.2ms +/- 0.2% 395.8ms +/- 0.1% significant cube: 1.013x as fast 135.6ms +/- 0.3% 133.9ms +/- 0.3% significant morph: *1.004x as slow* 132.4ms +/- 0.3% 132.9ms +/- 0.2% significant raytrace: 1.033x as fast 133.2ms +/- 0.2% 129.0ms +/- 0.0% significant access: 1.047x as fast 533.0ms +/- 0.3% 509.1ms +/- 0.1% significant binary-trees: 1.162x as fast 78.2ms +/- 0.4% 67.3ms +/- 0.5% significant fannkuch: 1.056x as fast 249.3ms +/- 0.3% 236.1ms +/- 0.1% significant nbody: - 147.7ms +/- 0.2% 147.8ms +/- 0.2% nsieve: ?? 57.8ms +/- 0.8% 57.9ms +/- 0.4% not conclusive: might be *1.002x as slow* bitops: 1.062x as fast 404.8ms +/- 0.4% 381.0ms +/- 0.1% significant 3bit-bits-in-byte: 1.101x as fast 81.5ms +/- 0.6% 74.0ms +/- 0.0% significant bits-in-byte: 1.109x as fast 104.8ms +/- 0.3% 94.5ms +/- 0.4% significant bitwise-and: 1.068x as fast 111.8ms +/- 1.1% 104.7ms +/- 0.3% significant nsieve-bits: *1.010x as slow* 106.7ms +/- 0.3% 107.8ms +/- 0.3% significant controlflow: 1.25x as fast 111.8ms +/- 0.3% 89.5ms +/- 0.6% significant recursive: 1.25x as fast 111.8ms +/- 0.3% 89.5ms +/- 0.6% significant crypto: 1.088x as fast 263.5ms +/- 0.2% 242.2ms +/- 0.3% significant aes: 1.006x as fast 80.2ms +/- 0.4% 79.7ms +/- 0.4% significant md5: 1.144x as fast 92.7ms +/- 0.4% 81.0ms +/- 0.4% significant sha1: 1.112x as fast 90.6ms +/- 0.4% 81.5ms +/- 0.5% significant date: 1.026x as fast 291.3ms +/- 0.1% 284.0ms +/- 0.2% significant format-tofte: 1.004x as fast 133.3ms +/- 0.3% 132.8ms +/- 0.3% significant format-xparb: 1.045x as fast 158.0ms +/- 0.0% 151.2ms +/- 0.2% significant math: 1.037x as fast 428.1ms +/- 0.3% 413.0ms +/- 0.2% significant cordic: 1.025x as fast 172.0ms +/- 0.5% 167.8ms +/- 0.4% significant partial-sums: - 163.2ms +/- 0.5% 162.8ms +/- 0.2% spectral-norm: 1.127x as fast 92.9ms +/- 0.4% 82.4ms +/- 0.4% significant regexp: *1.060x as slow* 207.4ms +/- 0.2% 219.8ms +/- 0.4% significant dna: *1.060x as slow* 207.4ms +/- 0.2% 219.8ms +/- 0.4% significant string: 1.053x as fast 680.6ms +/- 0.2% 646.1ms +/- 0.2% significant base64: 1.016x as fast 99.3ms +/- 0.6% 97.7ms +/- 0.5% significant fasta: 1.039x as fast 172.5ms +/- 0.5% 166.0ms +/- 0.3% significant tagcloud: 1.067x as fast 146.5ms +/- 0.5% 137.3ms +/- 0.3% significant unpack-code: 1.090x as fast 150.9ms +/- 0.3% 138.5ms +/- 0.3% significant validate-input: 1.045x as fast 111.4ms +/- 1.1% 106.6ms +/- 0.8% significant As you can see, it's a nice improvement across the board and an even better improvement on benchmarks that use a lot of function calls. The only test that is slightly mysterious is the regexp-dna test, because it doesn't have any user functions and thus shouldn't be getting a significant hit. It might be a code layout issue. The one thing I didn't do was attempt to annotate the syntax tree so that many cases of tear-off could be avoided by simply allocating the activation record on the JS heap in the first place. This might give a slight performance improvement. I didn't even bother including a ChangeLog at this point, because I know the style police are going to be all over me with this one. I apologize in advance if there is anything particularly bad in the code, but it is difficult to do some of the things in the patch in a completely clean way. I know a lot of people, myself included, want to see this landed soon, so I will try to be extremely responsive to any comments and suggestions from reviewers. Hopefully, we can get a final version in a few days.
Eric Seidel (no email)
Comment 12 2008-01-05 17:20:01 PST
Comment on attachment 18287 [details] Proposed patch Obviously the commented out printf()s will need to be removed. Also, the indenting is incorrect on things like ActivationImp::init, 4 spaces instead of 2. Single line ifs are two lines: if (!switchGlobal) exec->dynamicGlobalObject()->tearOffActivation(exec); Otherwise looks OK. mjs definitely should look at this. You should read: http://webkit.org/coding/coding-style.html
Cameron Zwarich (cpst)
Comment 13 2008-01-05 18:26:15 PST
Created attachment 18292 [details] Revised proposed patch (In reply to comment #12) > (From update of attachment 18287 [details] [edit]) > Obviously the commented out printf()s will need to be removed. Oops! Took those out. > Also, the > indenting is incorrect on things like ActivationImp::init, 4 spaces instead of > 2. There might be an indentation problem somewhere, due to the inconsistent indentation throughout JSC, but I used 2 spaces for ActivationImp::init. > Single line ifs are two lines: > if (!switchGlobal) exec->dynamicGlobalObject()->tearOffActivation(exec); Fixed. > You should read: http://webkit.org/coding/coding-style.html I've read it, but I still occasionally make some mistakes. Sorry.
Cameron Zwarich (cpst)
Comment 14 2008-01-05 19:15:19 PST
Created attachment 18293 [details] Revised proposed patch Some indentation fixes.
Darin Adler
Comment 15 2008-01-06 11:54:24 PST
Comment on attachment 18293 [details] Revised proposed patch 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.
Cameron Zwarich (cpst)
Comment 16 2008-01-06 15:43:58 PST
(In reply to comment #15) > (From update of attachment 18293 [details] [edit]) > I think this looks really good. Lets get it landed! Thanks. Maciej gave me some comments in the channel, so I've already made some of your requested changes. > 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. No problem. I can do that. > 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. Some header changes were made since I started working on this patch, so I can remove everything before ActivationImp.h. > 41 friend class StackActivation; > 42 friend class JSGlobalObject; > > Do we really need these friend declarations? We usually try to avoid them if > possible. Since the definition of ActivationImpData is private ActivationImp, in order for StackActivation to have one, it must be a friiend of ActivationImp. Either way, JSGlobalObject does, because d() is a private method of ActivationImp. Both of these conventions are consistent across the current code, so I didn't want to change them. > 44 using JSVariableObject::JSVariableObjectData; > > How does this "using" help us? I actually don't know. But I also wasn't the one to add it. I only copied it from function.h to ActivationImp.h > 46 struct ActivationImpData : public > JSVariableObject::JSVariableObjectData { > > Because of the "using" you don't need to qualify the JSVariableObjectData here > with the class name. I just removed the "using" instead. Is that better? > 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"? Because the class already exists in function.h and is called ActivationImpData. I didn't want to change the name of an existing class. > 80 bool isOnStack() { return d()->onStack; } > > Maybe this should be a const member? It can't be a member, because we can't make ActivationImp any bigger. > 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. Again, I did not introduce it. It was there before for ActivationImp, and seems to be an idiom in the code for anything that inherits from JSVariableObject, to cast JSVariableObject::d to its own class that inherits from JSVariableObjectData. > 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? I'll change it back. > 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. Sounds like it might be a good idea, but this patch is not really the place for the change, is it? > 3232 #include "SavedBuiltins.h" > 33 #include "ActivationImp.h" > 3334 #include "array_object.h" > > We normally keep these lists of includes sorted alphabetically. I know, but I was adding one file to an existing list that is already not alphabetical. I'll resort it. > 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? I could make another inline function that both of them call. I should also probably do the same with this code, duplicated between the two functions: if (!d()->activationCount) { ActivationStackNode* prev = d()->activations->prev; delete d()->activations; d()->activations = prev; d()->activationCount = activationStackSize; } > 59 JSVariableObjectData() { } > > 81 JSVariableObject() { } > > How are these empty constructors helpful? They are are all required because StackActivation has both a JSVariableObject and a JSVariableObjectData as members, and they need to be allocated without doing any initialization. > 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. It's only used twice and there still needs to be explicit tear-off elsewhere, so is it really worth it? > 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. I just copied the existing code, but I'll change it to a for loop. > 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. I'll make the change, but there should also be a better way to copy a Vector than iterating through the elements. > 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. getPropertySlot can now cause an activation record to get torn off when you ask for the slot for "arguments", so anything that asks for it must check to get the activation record again. I'll add it as a comment. > 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. That makes sense. > 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. I'll make the changes you wanted (besides the ones I still have questions about) and post a new patch soon.
Cameron Zwarich (cpst)
Comment 17 2008-01-06 18:44:15 PST
Created attachment 18307 [details] Revised proposed patch Here is a revised version that incorporates most of the changes Darin requested.
Maciej Stachowiak
Comment 18 2008-01-07 01:54:56 PST
Comment on attachment 18307 [details] Revised proposed patch I reviewed this. The style is looking pretty good, I made a few suggestions on IRC. The only showstopper is that this has a potential crashing bug when tearing off an activation that is not the top opne (which can only happen when getting the "arguments" property from a function object.
Darin Adler
Comment 19 2008-01-07 12:45:24 PST
(In reply to comment #16) > > 46 struct ActivationImpData : public > > JSVariableObject::JSVariableObjectData { > > > > Because of the "using" you don't need to qualify the JSVariableObjectData here > > with the class name. > > I just removed the "using" instead. Is that better? Yes, I think so. > > 80 bool isOnStack() { return d()->onStack; } > > > > Maybe this should be a const member? > > It can't be a member, because we can't make ActivationImp any bigger. I was referring to isOnStack(), not onStack. Making it a const member function instead of non-const. > > 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. > > Sounds like it might be a good idea, but this patch is not really the place for > the change, is it? Well, the class-specific work is new to your patch. The branch to check the codeType is new. That's why I raised the issue. It could be done in separate patch, yes, either before or after this change. But it is this change to the class that creates the need for it. > > 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. > > It's only used twice and there still needs to be explicit tear-off elsewhere, > so is it really worth it? Probably not. > > 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. > > I'll make the change, but there should also be a better way to copy a Vector > than iterating through the elements. There is. You can just use assignment to copy one vector to another. But the code above is marking a vector, not copying a vector. So I'm not sure what you mean. > > > 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. > > getPropertySlot can now cause an activation record to get torn off when you ask > for the slot for "arguments", so anything that asks for it must check to get > the activation record again. I'll add it as a comment. It seems pointless to put base into a local variable if it's immediately going to be invalidated. I would probably structure it a little differently.
Cameron Zwarich (cpst)
Comment 20 2008-01-08 07:09:28 PST
Created attachment 18327 [details] Revised proposed patch (In reply to comment #19) > > > 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. > > > > I'll make the change, but there should also be a better way to copy a Vector > > than iterating through the elements. > > There is. You can just use assignment to copy one vector to another. > > But the code above is marking a vector, not copying a vector. So I'm not sure > what you mean. Sorry. I think I was thinking about two things at once. > > > 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. > > > > getPropertySlot can now cause an activation record to get torn off when you ask > > for the slot for "arguments", so anything that asks for it must check to get > > the activation record again. I'll add it as a comment. > > It seems pointless to put base into a local variable if it's immediately going > to be invalidated. I would probably structure it a little differently. I structured it differently in the cases that don't use goto. Those could be rewritten as well, if you want, but I didn't want to change unrelated existing code. Here's a new version of the patch that corrects the tear-off behaviour when tearing off something that is not at the top of the activation stack, and includes a test case to check for this (the test returned 'undefined' before the change).
Darin Adler
Comment 21 2008-01-08 22:02:36 PST
Comment on attachment 18327 [details] Revised proposed patch I think you accidentally pasted the old patch again. This is not updated.
Darin Adler
Comment 22 2008-01-08 22:04:11 PST
Comment on attachment 18327 [details] Revised proposed patch On second thought, maybe there are some updates, but the changes I requested have not been done -- is this really the latest version?
Cameron Zwarich (cpst)
Comment 23 2008-01-08 22:04:26 PST
Created attachment 18344 [details] Revised proposed patch Oops. Here's the actual patch.
Darin Adler
Comment 24 2008-01-08 22:18:50 PST
Comment on attachment 18344 [details] Revised proposed patch 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.
Cameron Zwarich (cpst)
Comment 25 2008-01-08 22:23:04 PST
(In reply to comment #24) > (From update of attachment 18344 [details] [edit]) > This looks great. I am going to say review+, although if you decide to review > this you may want to clear the review flag. I cleared the flag because I might as well make these changes.
Cameron Zwarich (cpst)
Comment 26 2008-01-09 00:51:31 PST
Created attachment 18346 [details] Revised proposed patch Here it is.
Maciej Stachowiak
Comment 27 2008-01-11 16:11:36 PST
Comment on attachment 18346 [details] Revised proposed patch r=me
Maciej Stachowiak
Comment 28 2008-01-11 18:13:21 PST
Landed.
Note You need to log in before you can comment on or make changes to this bug.