Bug 178067 – [JSC] __proto__ getter should be fast

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

[Yusuke Suzuki]

__proto__ access is typically lowered into Call node in DFG, which is apparently not good.

[Yusuke Suzuki]

I think this is a bit critical since we typically use __proto__ accessor for ES6 class.

[Yusuke Suzuki]

SixSpeed/super.es6 becomes 3.4x faster in my working copy.

[Yusuke Suzuki]

IC should handle it too.

[Yusuke Suzuki]

Created attachment 323287 [details]
Patch

WIP

[Build Bot]

Attachment 323287 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:3872:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 1 in 40 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 323287 [details]
Patch

Attachment 323287 [details] did not pass jsc-ews (mac):
Output: http://webkit-queues.webkit.org/results/4810261

New failing tests:
stress/object-prototype-proto-accessors-should-throw-on-undefined-this.js.misc-ftl-no-cjit
stress/proxy-get-prototype-of.js.ftl-eager-no-cjit
stress/proxy-get-prototype-of.js.ftl-eager
stress/proxy-get-prototype-of.js.ftl-eager-no-cjit-b3o1

[Yusuke Suzuki]

Created attachment 323293 [details]
Patch

[Build Bot]

Attachment 323293 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:3872:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 1 in 46 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Yusuke Suzuki]

Created attachment 323295 [details]
Patch

[Build Bot]

Attachment 323295 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:3872:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 1 in 47 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Yusuke Suzuki]

Created attachment 323308 [details]
Patch

[Build Bot]

Attachment 323308 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:3872:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 1 in 47 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Saam Barati]

Comment on attachment 323308 [details]
Patch

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

r=me with some nits and a comment regarding IntrinsicGetters

> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:2706
> +            bool cannotFold = false;

style nit: I think these variables are easier to read if they say what they "can" do vs "cannot" do. So, I think it'd be easier if we called this "canFold" and flipped all the assignments to it.

> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:2726
> +            if (prototype && !cannotFold) {

so, for example, I think it's easier to read this as
prototype && canFold
instead of
prototype && !cannotFold

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:3060
> +        variant.structureSet().forEach([&] (Structure* structure) {

is this set guaranteed to be non-empty?

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:10874
> +        callOperation(operationGetPrototypeOf, resultRegs, JSValueRegs::payloadOnly(valueGPR));

Is it worth having a version of the function where the operand is already a JSObject*?

> Source/JavaScriptCore/jit/IntrinsicEmitter.cpp:141
> +        jit.loadValue(MacroAssembler::Address(baseGPR, sizeof(EncodedJSValue) * structure()->polyProtoOffset() + JSObject::offsetOfInlineStorage()), valueRegs);
> +        state.succeed();
> +        return;

I bet this is dead code since Repatch.cpp doesn't emit intrinsic getters with poly proto. The reason for this is that I didn't make it work with the typed array code. So if you want this to work with poly proto, I think you need to change things in Repatch.cpp to get it to emit these. You may want to disable it for typed arrays ATM. Or have canEmitIntrinsicGetter know if it's a poly proto access or not. The reason this isn't trivial to do is take this example:
```
function foo(o) {
    return o.__proto__;
}
let o = somethingPolyProto;
assert(o.__proto__ === Object.prototype);
assert(foo(o) === Object.prototype);
o.__proto__ = null;
assert(foo(o) === undefined)
```

I think your code would break on this case, since you're not verifying the prototype chain. So, I think it's worth skipping poly proto for now.

[Yusuke Suzuki]

Comment on attachment 323308 [details]
Patch

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

>> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:2706
>> +            bool cannotFold = false;
> 
> style nit: I think these variables are easier to read if they say what they "can" do vs "cannot" do. So, I think it'd be easier if we called this "canFold" and flipped all the assignments to it.

Changed, nice.

>> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:2726
>> +            if (prototype && !cannotFold) {
> 
> so, for example, I think it's easier to read this as
> prototype && canFold
> instead of
> prototype && !cannotFold

Yeah, make sense. Fixed.

>> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:3060
>> +        variant.structureSet().forEach([&] (Structure* structure) {
> 
> is this set guaranteed to be non-empty?

Even if this set is empty, it is ok because `prototype` becomes empty.
So, folding does not occur.
But to make it more readable, I've changed it to `bool canFold = !variant.structureSet().isEmpty();`.

>> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:10874
>> +        callOperation(operationGetPrototypeOf, resultRegs, JSValueRegs::payloadOnly(valueGPR));
> 
> Is it worth having a version of the function where the operand is already a JSObject*?

It seems that adding it makes this a bit simple. I've added this function.

>> Source/JavaScriptCore/jit/IntrinsicEmitter.cpp:141
>> +        return;
> 
> I bet this is dead code since Repatch.cpp doesn't emit intrinsic getters with poly proto. The reason for this is that I didn't make it work with the typed array code. So if you want this to work with poly proto, I think you need to change things in Repatch.cpp to get it to emit these. You may want to disable it for typed arrays ATM. Or have canEmitIntrinsicGetter know if it's a poly proto access or not. The reason this isn't trivial to do is take this example:
> ```
> function foo(o) {
>     return o.__proto__;
> }
> let o = somethingPolyProto;
> assert(o.__proto__ === Object.prototype);
> assert(foo(o) === Object.prototype);
> o.__proto__ = null;
> assert(foo(o) === undefined)
> ```
> 
> I think your code would break on this case, since you're not verifying the prototype chain. So, I think it's worth skipping poly proto for now.

Yes, this is dead code since Repatch do not emit IntrinsicGetter IC for poly proto structure.
I added this code since this should be placed here once poly proto support is enabled for this intrinsic getter thing.
And at that time, the implementation would become like this here since poly proto check should be already done when reaching this.

I'll add FIXME comment here with the URL to https://bugs.webkit.org/show_bug.cgi?id=177318.

[Yusuke Suzuki]

Committed r223523: <https://trac.webkit.org/changeset/223523>

[Radar WebKit Bug Importer]

<rdar://problem/35027592>

[Yusuke Suzuki]

This improves ARES-6/ML average worst case & steady state :)
This is because this test frequently accesses __proto__ when calling super constructor in class's constructor.
https://arewefastyet.com/#machine=29&view=single&suite=ares6&subtest=ML-averageWorstCase
https://arewefastyet.com/#machine=29&view=single&suite=ares6&subtest=ML-steadyState

[Yusuke Suzuki]

(In reply to Yusuke Suzuki from comment #17)
> This improves ARES-6/ML average worst case & steady state :)
> This is because this test frequently accesses __proto__ when calling super
> constructor in class's constructor.
> https://arewefastyet.com/#machine=29&view=single&suite=ares6&subtest=ML-
> averageWorstCase
> https://arewefastyet.com/#machine=29&view=single&suite=ares6&subtest=ML-
> steadyState

This should be further improved by https://bugs.webkit.org/show_bug.cgi?id=178064

[Saam Barati]

Comment on attachment 323308 [details]
Patch

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

>>> Source/JavaScriptCore/jit/IntrinsicEmitter.cpp:141
>>> +        return;
>> 
>> I bet this is dead code since Repatch.cpp doesn't emit intrinsic getters with poly proto. The reason for this is that I didn't make it work with the typed array code. So if you want this to work with poly proto, I think you need to change things in Repatch.cpp to get it to emit these. You may want to disable it for typed arrays ATM. Or have canEmitIntrinsicGetter know if it's a poly proto access or not. The reason this isn't trivial to do is take this example:
>> ```
>> function foo(o) {
>>     return o.__proto__;
>> }
>> let o = somethingPolyProto;
>> assert(o.__proto__ === Object.prototype);
>> assert(foo(o) === Object.prototype);
>> o.__proto__ = null;
>> assert(foo(o) === undefined)
>> ```
>> 
>> I think your code would break on this case, since you're not verifying the prototype chain. So, I think it's worth skipping poly proto for now.
> 
> Yes, this is dead code since Repatch do not emit IntrinsicGetter IC for poly proto structure.
> I added this code since this should be placed here once poly proto support is enabled for this intrinsic getter thing.
> And at that time, the implementation would become like this here since poly proto check should be already done when reaching this.
> 
> I'll add FIXME comment here with the URL to https://bugs.webkit.org/show_bug.cgi?id=177318.

Please remove this code. It's incorrect, and the way we should add the FIXME is inside "canEmitIntrinsicGetter" with a description of the problem. That function should return false if hasPolyProto() is true. My example above explains why doing this isn't trivial, since we don't do Structure transitions when setting the prototype on a poly proto object.

[Ryan Haddad]

Reverted r223523 for reason:

A test for this change is failing on debug JSC bots.

Committed r223584: <https://trac.webkit.org/changeset/223584>

[Ryan Haddad]

*** Bug 178399 has been marked as a duplicate of this bug. ***

[Yusuke Suzuki]

Committed r223594: <https://trac.webkit.org/changeset/223594>

[Yusuke Suzuki]

Comment on attachment 323308 [details]
Patch

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

>>>> Source/JavaScriptCore/jit/IntrinsicEmitter.cpp:141
>>>> +        return;
>>> 
>>> I bet this is dead code since Repatch.cpp doesn't emit intrinsic getters with poly proto. The reason for this is that I didn't make it work with the typed array code. So if you want this to work with poly proto, I think you need to change things in Repatch.cpp to get it to emit these. You may want to disable it for typed arrays ATM. Or have canEmitIntrinsicGetter know if it's a poly proto access or not. The reason this isn't trivial to do is take this example:
>>> ```
>>> function foo(o) {
>>>     return o.__proto__;
>>> }
>>> let o = somethingPolyProto;
>>> assert(o.__proto__ === Object.prototype);
>>> assert(foo(o) === Object.prototype);
>>> o.__proto__ = null;
>>> assert(foo(o) === undefined)
>>> ```
>>> 
>>> I think your code would break on this case, since you're not verifying the prototype chain. So, I think it's worth skipping poly proto for now.
>> 
>> Yes, this is dead code since Repatch do not emit IntrinsicGetter IC for poly proto structure.
>> I added this code since this should be placed here once poly proto support is enabled for this intrinsic getter thing.
>> And at that time, the implementation would become like this here since poly proto check should be already done when reaching this.
>> 
>> I'll add FIXME comment here with the URL to https://bugs.webkit.org/show_bug.cgi?id=177318.
> 
> Please remove this code. It's incorrect, and the way we should add the FIXME is inside "canEmitIntrinsicGetter" with a description of the problem. That function should return false if hasPolyProto() is true. My example above explains why doing this isn't trivial, since we don't do Structure transitions when setting the prototype on a poly proto object.

OK, I've changed canEmitIntrinsicGetter part to check hasPolyProto, and remove this code for poly proto thing.
But I believe this code is not executed right now because our Repatch.cpp do not emit intrinsic getters for poly proto structures, correct?

[Ryosuke Niwa]

Looks like this patch broke builds everywhere:
/Volumes/Data/slave/elcapitan-release/build/WebKitBuild/Release/JavaScriptCore.framework/PrivateHeaders/JSGlobalObject.h:26:10: fatal error: 'BooleanPrototype.h' file not found
#include "BooleanPrototype.h"
         ^
1 error generated.

[Ryosuke Niwa]

(In reply to Ryosuke Niwa from comment #24)
> Looks like this patch broke builds everywhere:
> /Volumes/Data/slave/elcapitan-release/build/WebKitBuild/Release/
> JavaScriptCore.framework/PrivateHeaders/JSGlobalObject.h:26:10: fatal error:
> 'BooleanPrototype.h' file not found
> #include "BooleanPrototype.h"
>          ^
> 1 error generated.

Fixed it in https://trac.webkit.org/changeset/223595.

[Yusuke Suzuki]

(In reply to Ryosuke Niwa from comment #25)
> (In reply to Ryosuke Niwa from comment #24)
> > Looks like this patch broke builds everywhere:
> > /Volumes/Data/slave/elcapitan-release/build/WebKitBuild/Release/
> > JavaScriptCore.framework/PrivateHeaders/JSGlobalObject.h:26:10: fatal error:
> > 'BooleanPrototype.h' file not found
> > #include "BooleanPrototype.h"
> >          ^
> > 1 error generated.
> 
> Fixed it in https://trac.webkit.org/changeset/223595.

Oops, thanks.

[Saam Barati]

(In reply to Yusuke Suzuki from comment #23)
> Comment on attachment 323308 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=323308&action=review
> 
> >>>> Source/JavaScriptCore/jit/IntrinsicEmitter.cpp:141
> >>>> +        return;
> >>> 
> >>> I bet this is dead code since Repatch.cpp doesn't emit intrinsic getters with poly proto. The reason for this is that I didn't make it work with the typed array code. So if you want this to work with poly proto, I think you need to change things in Repatch.cpp to get it to emit these. You may want to disable it for typed arrays ATM. Or have canEmitIntrinsicGetter know if it's a poly proto access or not. The reason this isn't trivial to do is take this example:
> >>> ```
> >>> function foo(o) {
> >>>     return o.__proto__;
> >>> }
> >>> let o = somethingPolyProto;
> >>> assert(o.__proto__ === Object.prototype);
> >>> assert(foo(o) === Object.prototype);
> >>> o.__proto__ = null;
> >>> assert(foo(o) === undefined)
> >>> ```
> >>> 
> >>> I think your code would break on this case, since you're not verifying the prototype chain. So, I think it's worth skipping poly proto for now.
> >> 
> >> Yes, this is dead code since Repatch do not emit IntrinsicGetter IC for poly proto structure.
> >> I added this code since this should be placed here once poly proto support is enabled for this intrinsic getter thing.
> >> And at that time, the implementation would become like this here since poly proto check should be already done when reaching this.
> >> 
> >> I'll add FIXME comment here with the URL to https://bugs.webkit.org/show_bug.cgi?id=177318.
> > 
> > Please remove this code. It's incorrect, and the way we should add the FIXME is inside "canEmitIntrinsicGetter" with a description of the problem. That function should return false if hasPolyProto() is true. My example above explains why doing this isn't trivial, since we don't do Structure transitions when setting the prototype on a poly proto object.
> 
> OK, I've changed canEmitIntrinsicGetter part to check hasPolyProto, and
> remove this code for poly proto thing.
> But I believe this code is not executed right now because our Repatch.cpp do
> not emit intrinsic getters for poly proto structures, correct?
This code may or may not be executed, but at least it's correct. The previous implementation of just loading the poly proto prototype was just wrong code, because we'd need to verify the prototype chain has the __proto__ getter on it.

[Saam Barati]

(In reply to Saam Barati from comment #27)
> (In reply to Yusuke Suzuki from comment #23)
> > Comment on attachment 323308 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=323308&action=review
> > 
> > >>>> Source/JavaScriptCore/jit/IntrinsicEmitter.cpp:141
> > >>>> +        return;
> > >>> 
> > >>> I bet this is dead code since Repatch.cpp doesn't emit intrinsic getters with poly proto. The reason for this is that I didn't make it work with the typed array code. So if you want this to work with poly proto, I think you need to change things in Repatch.cpp to get it to emit these. You may want to disable it for typed arrays ATM. Or have canEmitIntrinsicGetter know if it's a poly proto access or not. The reason this isn't trivial to do is take this example:
> > >>> ```
> > >>> function foo(o) {
> > >>>     return o.__proto__;
> > >>> }
> > >>> let o = somethingPolyProto;
> > >>> assert(o.__proto__ === Object.prototype);
> > >>> assert(foo(o) === Object.prototype);
> > >>> o.__proto__ = null;
> > >>> assert(foo(o) === undefined)
> > >>> ```
> > >>> 
> > >>> I think your code would break on this case, since you're not verifying the prototype chain. So, I think it's worth skipping poly proto for now.
> > >> 
> > >> Yes, this is dead code since Repatch do not emit IntrinsicGetter IC for poly proto structure.
> > >> I added this code since this should be placed here once poly proto support is enabled for this intrinsic getter thing.
> > >> And at that time, the implementation would become like this here since poly proto check should be already done when reaching this.
> > >> 
> > >> I'll add FIXME comment here with the URL to https://bugs.webkit.org/show_bug.cgi?id=177318.
> > > 
> > > Please remove this code. It's incorrect, and the way we should add the FIXME is inside "canEmitIntrinsicGetter" with a description of the problem. That function should return false if hasPolyProto() is true. My example above explains why doing this isn't trivial, since we don't do Structure transitions when setting the prototype on a poly proto object.
> > 
> > OK, I've changed canEmitIntrinsicGetter part to check hasPolyProto, and
> > remove this code for poly proto thing.
> > But I believe this code is not executed right now because our Repatch.cpp do
> > not emit intrinsic getters for poly proto structures, correct?
> This code may or may not be executed, but at least it's correct. The
> previous implementation of just loading the poly proto prototype was just
> wrong code, because we'd need to verify the prototype chain has the
> __proto__ getter on it.

That said, it's also wrong if the base object point to a parent object that has poly proto, that's why Repatch itself does the check. I'm just noting it's also wrong for the base object. We just need to teach intrinsic getters about poly proto. That should be pretty easy to do. Also making __proto__ getter fast is a good reason to do this, since the only reason for doing this before was if somebody changed the TypedArray prototype hierarchy, which is unlikely to happen in real code. Accessing __proto__ is common in real code.