Bug 178067 - [JSC] __proto__ getter should be fast
Summary: [JSC] __proto__ getter should be fast
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Yusuke Suzuki
URL:
Keywords: InRadar
: 178399 (view as bug list)
Depends on:
Blocks: 178064
  Show dependency treegraph
 
Reported: 2017-10-08 01:30 PDT by Yusuke Suzuki
Modified: 2017-10-18 15:55 PDT (History)
11 users (show)

See Also:


Attachments
Patch (66.46 KB, patch)
2017-10-10 01:31 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (75.51 KB, patch)
2017-10-10 02:55 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (76.47 KB, patch)
2017-10-10 03:34 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (76.47 KB, patch)
2017-10-10 07:42 PDT, Yusuke Suzuki
sbarati: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yusuke Suzuki 2017-10-08 01:30:10 PDT
__proto__ access is typically lowered into Call node in DFG, which is apparently not good.
Comment 1 Yusuke Suzuki 2017-10-08 01:30:37 PDT
I think this is a bit critical since we typically use __proto__ accessor for ES6 class.
Comment 2 Yusuke Suzuki 2017-10-08 03:20:15 PDT
SixSpeed/super.es6 becomes 3.4x faster in my working copy.
Comment 3 Yusuke Suzuki 2017-10-08 03:20:37 PDT
IC should handle it too.
Comment 4 Yusuke Suzuki 2017-10-10 01:31:16 PDT
Created attachment 323287 [details]
Patch

WIP
Comment 5 Build Bot 2017-10-10 01:33:33 PDT
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.
Comment 6 Build Bot 2017-10-10 02:16:02 PDT
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
Comment 7 Yusuke Suzuki 2017-10-10 02:55:12 PDT
Created attachment 323293 [details]
Patch
Comment 8 Build Bot 2017-10-10 02:57:55 PDT
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.
Comment 9 Yusuke Suzuki 2017-10-10 03:34:02 PDT
Created attachment 323295 [details]
Patch
Comment 10 Build Bot 2017-10-10 03:36:51 PDT
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.
Comment 11 Yusuke Suzuki 2017-10-10 07:42:13 PDT
Created attachment 323308 [details]
Patch
Comment 12 Build Bot 2017-10-10 07:45:02 PDT
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.
Comment 13 Saam Barati 2017-10-16 11:33:32 PDT
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.
Comment 14 Yusuke Suzuki 2017-10-17 04:54:20 PDT
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.
Comment 15 Yusuke Suzuki 2017-10-17 05:02:16 PDT
Committed r223523: <https://trac.webkit.org/changeset/223523>
Comment 16 Radar WebKit Bug Importer 2017-10-17 05:03:13 PDT
<rdar://problem/35027592>
Comment 17 Yusuke Suzuki 2017-10-17 06:28:06 PDT
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
Comment 18 Yusuke Suzuki 2017-10-17 06:31:41 PDT
(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
Comment 19 Saam Barati 2017-10-17 12:12:32 PDT
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.
Comment 20 Ryan Haddad 2017-10-17 16:34:35 PDT
Reverted r223523 for reason:

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

Committed r223584: <https://trac.webkit.org/changeset/223584>
Comment 21 Ryan Haddad 2017-10-17 16:36:05 PDT
*** Bug 178399 has been marked as a duplicate of this bug. ***
Comment 22 Yusuke Suzuki 2017-10-18 00:13:06 PDT
Committed r223594: <https://trac.webkit.org/changeset/223594>
Comment 23 Yusuke Suzuki 2017-10-18 00:16:08 PDT
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?
Comment 24 Ryosuke Niwa 2017-10-18 00:42:40 PDT
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.
Comment 25 Ryosuke Niwa 2017-10-18 01:12:40 PDT
(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.
Comment 26 Yusuke Suzuki 2017-10-18 01:44:18 PDT
(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.
Comment 27 Saam Barati 2017-10-18 15:54:22 PDT
(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.
Comment 28 Saam Barati 2017-10-18 15:55:52 PDT
(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.