Bug 178067

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

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

IC should handle it too.

Created attachment 323287 [details] Patch WIP

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 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

Created attachment 323293 [details] Patch

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.

Created attachment 323295 [details] Patch

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.

Created attachment 323308 [details] Patch

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 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 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.

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

<rdar://problem/35027592>

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

(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 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.

Reverted r223523 for reason: A test for this change is failing on debug JSC bots. Committed r223584: <https://trac.webkit.org/changeset/223584>

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

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

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?

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.

(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.

(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.

(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.

(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.