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

[bugzilla-daemon at webkit.org]

Cameron Zwarich (cpst) <cwzwarich at uwaterloo.ca> has asked  for review:
Bug 16202: Optimize allocation of ActivationImp objects
http://bugs.webkit.org/show_bug.cgi?id=16202

Attachment 18327: Revised proposed patch
http://bugs.webkit.org/attachment.cgi?id=18327&action=edit

------- Additional Comments from Cameron Zwarich (cpst)
<cwzwarich at uwaterloo.ca>
(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).