Bug 16202 – Optimize allocation of ActivationImp objects

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Cameron Zwarich (cpst)]

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.

[Eric Seidel (no email)]

Hum... so now all we need is the patch. :)

[Maciej Stachowiak]

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]

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)]

(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]

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)]

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)]

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)]

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)]

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)]

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)]

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 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)]

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)]

Created attachment 18293 [details]
Revised proposed patch

Some indentation fixes.

[Darin Adler]

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)]

(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)]

Created attachment 18307 [details]
Revised proposed patch

Here is a revised version that incorporates most of the changes Darin requested.

[Maciej Stachowiak]

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]

(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)]

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 on attachment 18327 [details]
Revised proposed patch

I think you accidentally pasted the old patch again. This is not updated.

[Darin Adler]

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)]

Created attachment 18344 [details]
Revised proposed patch

Oops. Here's the actual patch.

[Darin Adler]

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)]

(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)]

Created attachment 18346 [details]
Revised proposed patch

Here it is.

[Maciej Stachowiak]

Comment on attachment 18346 [details]
Revised proposed patch

r=me

[Maciej Stachowiak]

Landed.