[webkit-reviews] review requested: [Bug 16202] Optimize allocation
of ActivationImp objects : [Attachment 18327] Revised proposed patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Tue Jan 8 07:09:28 PST 2008
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).
More information about the webkit-reviews
mailing list