Bug 130857 – Avoid fetching JSObject::structure() repeatedly in putDirectInternal.

• Component: JavaScriptCore
• Status: RESOLVED WONTFIX

[Andreas Kling]

Avoid fetching JSObject::structure() repeatedly in putDirectInternal.

[Andreas Kling]

Created attachment 227974 [details]
Patch

[Mark Hahnenberg]

Comment on attachment 227974 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=227974&action=review

r=me

> Source/JavaScriptCore/runtime/JSObject.h:1346
> +    if (Structure* newStructure = Structure::addPropertyTransitionToExistingStructure(this->structure(), propertyName, attributes, specificFunction, offset)) {

s/this->structure()/structure/ right?

[Andreas Kling]

Committed r166376: <http://trac.webkit.org/changeset/166376>

[Filip Pizlo]

Comment on attachment 227974 [details]
Patch

This really should have asserted, in all of the places that you removed a load of the structure, that the structure didn't change.  That's very important since there are subtle ways in which things could *become* effectful and obviously this code could break if that happened.

[Andreas Kling]

(In reply to comment #4)
> (From update of attachment 227974 [details])
> This really should have asserted, in all of the places that you removed a load of the structure, that the structure didn't change.  That's very important since there are subtle ways in which things could *become* effectful and obviously this code could break if that happened.

Sure, I'll slap that in there.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 130887

[Andreas Kling]

(In reply to comment #5)
> (In reply to comment #4)
> > (From update of attachment 227974 [details] [details])
> > This really should have asserted, in all of the places that you removed a load of the structure, that the structure didn't change.  That's very important since there are subtle ways in which things could *become* effectful and obviously this code could break if that happened.
> 
> Sure, I'll slap that in there.

Ended up rolling this out instead. I had misread the profile that lead to making this change.