Bug 177056 – [DFG] Remove ToThis more aggressively

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Yusuke Suzuki]

ToThis is rarely overridden. And toThis functions are very limited. We can check toThis function in AI and perform appropriate folding.

[Yusuke Suzuki]

Created attachment 321048 [details]
Patch

[Saam Barati]

Comment on attachment 321048 [details]
Patch

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

r=me

Can you also add a test where we change globalThisValue after we've already compiled the code, that way we ensure that we're not trying to completely constant fold the result of GetGlobalThis?

> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:174
> +        bool allStructureIsScope = !valueForNode.m_structure.isClear();

let's name this: allStructuresAreScope or allStructuresAreJSScope

> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:188
> +            if (!(type.isObject() && type.overridesToThis() && structure->classInfo()->methodTable.toThis == JSScope::info()->methodTable.toThis))

Why the three conditions? Doesn't (structure->classInfo()->methodTable.toThis == JSScope::info()->methodTable.toThis) suffice for your needs?
I'd maybe just write this:
``allStructuresAreScope &= structure->classInfo()->methodTable.toThis == JSScope::info()->methodTable.toThis``

[Yusuke Suzuki]

Comment on attachment 321048 [details]
Patch

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

I think testing it is difficult... Currently, we have two chances to see this issue,

1. JSC APIs

JSC APIs allows us to resetPrototype, which changes GlobalThis.

2. Touching JSGlobalObject in DFG before calling finishCreation()

If JSGlobalObject's initialization / JSDOMGlobalObject (and other childrens) invoke DFG in its initialization sequence, we may see that globalThis with nullptr.

I think testing them is difficult. I think (1) should be removed because JSGlobalObject is now immutable prototype exotic object.
And seconds thing relies on initialization of JSGlobalObject (and children).

I don't think we should restrict DFG invocation in JSGlobalObject initialization: it is useful to use JS things if some critical part of JSGlobalObject is initialized. Currently, globalThis is set after all the initialization finishes.

We eventually want to remove this replacing globalThis feature from JSC.

>> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:174
>> +        bool allStructureIsScope = !valueForNode.m_structure.isClear();
> 
> let's name this: allStructuresAreScope or allStructuresAreJSScope

Changed it to allStructuresAreJSScope.

>> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:188
>> +            if (!(type.isObject() && type.overridesToThis() && structure->classInfo()->methodTable.toThis == JSScope::info()->methodTable.toThis))
> 
> Why the three conditions? Doesn't (structure->classInfo()->methodTable.toThis == JSScope::info()->methodTable.toThis) suffice for your needs?
> I'd maybe just write this:
> ``allStructuresAreScope &= structure->classInfo()->methodTable.toThis == JSScope::info()->methodTable.toThis``

Oh, nice. Fixed.

[Yusuke Suzuki]

Committed r222143: <http://trac.webkit.org/changeset/222143>

[Radar WebKit Bug Importer]

<rdar://problem/34693252>