The current implementation is using the instruction op_put_by_id_with_this to handle super setter bindings correctly. This operand is just a call to slow path which means that setters called from super aren't optimized to use IC or accessor inlining. To unify these opcodes, we need to add one more operand on op_put_by_id to store the this reference and teach the following layers and ICs that base and this operands sometimes aren't the same.
Created attachment 305425 [details] WIP - Proposed Patch It is a WIP for this Patch. I'm missing the ChangeLog and create some micro benchmarks as well. In this patch I'm not enabling PutByIdWithThis IC for 32-bit architectures, because there is no sufficient GPRRegs to handle base, this and value and scratch at the same time.
Attachment 305425 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:9907: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/JavaScriptCore/jit/JITInlineCacheGenerator.cpp:175: Wrong number of spaces before statement. (expected: 8) [whitespace/indent] [4] ERROR: Source/JavaScriptCore/jit/JITInlineCacheGenerator.h:144: The parameter name "valueRegs" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:4564: An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement. [readability/control_flow] [4] Total errors found: 4 in 22 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 305425 [details] WIP - Proposed Patch Attachment 305425 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/3417323 New failing tests: fast/css/getComputedStyle/computed-style-font-family.html
Created attachment 305466 [details] Archive of layout-test-results from ews125 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews125 Port: ios-simulator-wk2 Platform: Mac OS X 10.11.6
Created attachment 305470 [details] WIP - Proposed Patch I noticed that IC was not being properly handled in put_by_id_with_this in the last patch because slot object wasn't properly configured in ```ordinarySetSlow```. Now in this version I'm proposing a change that configure the slot properly and make possible IC PuByIdWithThis. The results seem promising and I'm posting them here soon.
Attachment 305470 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/runtime/JSObject.h:1541: The parameter name "slot" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:9907: Place brace on its own line for function definitions. [whitespace/braces] [4] ERROR: Source/JavaScriptCore/jit/JITInlineCacheGenerator.cpp:175: Wrong number of spaces before statement. (expected: 8) [whitespace/indent] [4] ERROR: Source/JavaScriptCore/jit/JITInlineCacheGenerator.h:144: The parameter name "valueRegs" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:4564: An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement. [readability/control_flow] [4] Total errors found: 5 in 27 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 305471 [details] WIP - Benchmarks This is the last run I've made before the last Patch. I still need to update it using the last Patch uploaded.It is ~6x faster into a new micro benchmark that I've written named "super-setter". However, there are some regressions that I don't like. Investigating how to fix them.
Created attachment 305541 [details] Patch
Attachment 305541 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/runtime/JSObject.h:1541: The parameter name "slot" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/jit/JITInlineCacheGenerator.cpp:175: Wrong number of spaces before statement. (expected: 8) [whitespace/indent] [4] ERROR: Source/JavaScriptCore/jit/JITInlineCacheGenerator.h:144: The parameter name "valueRegs" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 3 in 29 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 305938 [details] Proposed Patch This version is fixing the previous case. In the previous case the IC was not being enabled when the property wasn't accessor.
Attachment 305938 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/jit/JITInlineCacheGenerator.cpp:175: Wrong number of spaces before statement. (expected: 8) [whitespace/indent] [4] ERROR: Source/JavaScriptCore/jit/JITInlineCacheGenerator.h:144: The parameter name "valueRegs" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 2 in 30 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 305939 [details] Benchmark This is the new Benchmark from last Patch version. Finally the overall result is neutral and there is no regression into micro benchmarks with good win into put-by-id-with-this cases. super-get-by-id-with-this-monomorphic ^ definitely 1.1759x faster super-get-by-id-with-this-polymorphic ^ definitely 1.2914x faster super-set-value ^ definitely 7.4017x faster super-setter ^ definitely 6.1591x faster
Ping review
Comment on attachment 305938 [details] Proposed Patch View in context: https://bugs.webkit.org/attachment.cgi?id=305938&action=review mostly LGTM, just a couple comments. I also think I found a bug. > Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:1912 > +#if USE(JSVALUE64_32) > + case PutByIdWithThis: > +#endif This is wrong. I believe it should be JSVALUE32_64. I think it's simpler if you just leave the 32-bit version in the code above, and have the USE(JSVALUE64) over a smaller range. > Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:4591 > + JITCompiler::JumpList notCellList; > + notCellList.append(m_jit.branchIfNotCell(JSValueRegs(baseGPR))); > + notCellList.append(m_jit.branchIfNotCell(JSValueRegs(thisGPR))); Looks like a bug that you don't use this after appending to it. Please add tests. Should be an Insta crash. That said, the more I think about it, are there any scenarios where |this|/base won't be a cell? > Source/JavaScriptCore/jit/Repatch.cpp:353 > + if (UNLIKELY(putKind == WithThis)) I don't think you need the UNLIKELY here. It might be the case that users do use this a lot.
Comment on attachment 305938 [details] Proposed Patch View in context: https://bugs.webkit.org/attachment.cgi?id=305938&action=review >> Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:4591 >> + notCellList.append(m_jit.branchIfNotCell(JSValueRegs(thisGPR))); > > Looks like a bug that you don't use this after appending to it. Please add tests. Should be an Insta crash. > > That said, the more I think about it, are there any scenarios where |this|/base won't be a cell? Oops. It should be a cachedPutByIdWithThis parameter. However, I'm following your thinking too. __proto__ can be "null" or "object". When it is null, ```super.something``` will fail as TypeError. (https://tc39.github.io/ecma262/#sec-set-object.prototype.__proto__). If I'm not missing something, either base/this are always Cell. >> Source/JavaScriptCore/jit/Repatch.cpp:353 >> + if (UNLIKELY(putKind == WithThis)) > > I don't think you need the UNLIKELY here. It might be the case that users do use this a lot. Done. > Source/JavaScriptCore/runtime/JSObjectInlines.h:205 > + if (UNLIKELY(isThisValueAltered(slot, thisObject) && slot.context() != PutPropertySlot::PutByIdWithThis)) Just got a case where this condition is not ok. Investigating a properly way to set slot properly here.
Created attachment 306287 [details] WIP - Patch Just found a bug in this approach to enable IC. Sending this to see the tests. A new Patch is coming soon.
Attachment 306287 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/jit/JITInlineCacheGenerator.cpp:175: Wrong number of spaces before statement. (expected: 8) [whitespace/indent] [4] ERROR: Source/JavaScriptCore/jit/JITInlineCacheGenerator.h:144: The parameter name "valueRegs" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 2 in 30 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 308185 [details] Proposed Patch This Version is enabling put_by_id_with_this for setter and value cases. Here are some benchmarks improved with this patch: super-set-value ^ definitely 9.2902x faster super-setter ^ definitely 5.9915x faster untyped-string-from-char-code ^ definitely 1.0725x faster super-get-by-id-with-this-monomorphic ^ definitely 1.1543x faster
Attachment 308185 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/runtime/JSObject.h:1542: The parameter name "slot" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/jit/JITInlineCacheGenerator.cpp:175: Wrong number of spaces before statement. (expected: 8) [whitespace/indent] [4] ERROR: Source/JavaScriptCore/jit/JITInlineCacheGenerator.h:144: The parameter name "valueRegs" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 3 in 34 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 308185 [details] Proposed Patch Attachment 308185 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/3606647 New failing tests: webrtc/datachannel/basic.html
Created attachment 308196 [details] Archive of layout-test-results from ews102 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews102 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Comment on attachment 308185 [details] Proposed Patch Attachment 308185 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/3606700 New failing tests: fast/forms/file/intrinsic-min-width-overrides-width.html
Created attachment 308207 [details] Archive of layout-test-results from ews122 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews122 Port: ios-simulator-wk2 Platform: Mac OS X 10.11.6
Created attachment 308242 [details] Proposed Patch Fixing some NITs, rebased with upstream code and re-triggering EWS.
Attachment 308242 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/runtime/JSObject.h:1542: The parameter name "slot" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/jit/JITInlineCacheGenerator.cpp:175: Wrong number of spaces before statement. (expected: 8) [whitespace/indent] [4] ERROR: Source/JavaScriptCore/jit/JITInlineCacheGenerator.h:144: The parameter name "valueRegs" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 3 in 34 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 308242 [details] Proposed Patch View in context: https://bugs.webkit.org/attachment.cgi?id=308242&action=review I'll keep looking at the rest of the patch, but some basic comments for now. > Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:1926 > +#if USE(JSVALUE64) > + { > + fixEdge<CellUse>(node->child1()); > + fixEdge<CellUse>(node->child2()); > + break; > + } > +#endif Style: This seems slightly too tricky. I'd just have this be its own case, with a "break" both for 64/32 bit platforms. > Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:249 > +void SpeculativeJIT::cachedPutByIdWithThis(CodeOrigin codeOrigin, GPRReg baseGPR, GPRReg valueGPR, GPRReg thisGPR, GPRReg scratchGPR, unsigned identifierNumber, JITCompiler::JumpList slowPathTargets, SpillRegistersMode spillMode) There is only one caller here, which never provides spillmode, please remove the parameter. Also, since this is a function w/ a single caller, I'd just remove this function alltogether and just inline it at the only call site. > Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:2944 > + if (m_node->child1().useKind() == CellUse && m_node->child2().useKind() == CellUse) > + putByIdWithThis(lowJSValue(m_node->child3()), lowCell(m_node->child1()), lowCell(m_node->child2())); You unconditionally speculate CellUse in FixupPhase, but then check here if it's not CellUse. Perhaps FixupPhase's speculation should be conditional? Otherwise, that "else" branch is most likely dead code.
Created attachment 308330 [details] Proposed Patch Addressing Saam's review
Attachment 308330 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/runtime/JSObject.h:1542: The parameter name "slot" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/jit/JITInlineCacheGenerator.cpp:175: Wrong number of spaces before statement. (expected: 8) [whitespace/indent] [4] ERROR: Source/JavaScriptCore/jit/JITInlineCacheGenerator.h:144: The parameter name "valueRegs" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 3 in 34 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 308242 [details] Proposed Patch View in context: https://bugs.webkit.org/attachment.cgi?id=308242&action=review >> Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:1926 >> +#endif > > Style: This seems slightly too tricky. I'd just have this be its own case, with a "break" both for 64/32 bit platforms. Done. >> Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:249 >> +void SpeculativeJIT::cachedPutByIdWithThis(CodeOrigin codeOrigin, GPRReg baseGPR, GPRReg valueGPR, GPRReg thisGPR, GPRReg scratchGPR, unsigned identifierNumber, JITCompiler::JumpList slowPathTargets, SpillRegistersMode spillMode) > > There is only one caller here, which never provides spillmode, please remove the parameter. Also, since this is a function w/ a single caller, I'd just remove this function alltogether and just inline it at the only call site. Sounds better. >> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:2944 >> + putByIdWithThis(lowJSValue(m_node->child3()), lowCell(m_node->child1()), lowCell(m_node->child2())); > > You unconditionally speculate CellUse in FixupPhase, but then check here if it's not CellUse. Perhaps FixupPhase's speculation should be conditional? Otherwise, that "else" branch is most likely dead code. Oops. Removed from DFG and forgot it here.
Created attachment 308823 [details] proposed Patch Rebasing with master and ping for review.
Attachment 308823 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/runtime/JSObject.h:1544: The parameter name "slot" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/jit/JITInlineCacheGenerator.cpp:175: Wrong number of spaces before statement. (expected: 8) [whitespace/indent] [4] ERROR: Source/JavaScriptCore/jit/JITInlineCacheGenerator.h:144: The parameter name "valueRegs" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 3 in 34 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 308823 [details] proposed Patch View in context: https://bugs.webkit.org/attachment.cgi?id=308823&action=review Some comments below. Much of this looks good, but I think there are still some bugs to work out. > Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:4795 > + CallSiteIndex callSite = m_jit.recordCallSiteAndGenerateExceptionHandlingOSRExitIfNeeded(node->origin.semantic, m_stream->size()); > + > + JITPutByIdWithThisGenerator gen( > + m_jit.codeBlock(), node->origin.semantic, callSite, this->usedRegisters(), JSValueRegs(valueGPR), JSValueRegs(baseGPR), > + JSValueRegs(thisGPR), scratchGPR, m_jit.ecmaModeFor(node->origin.semantic)); > + > + gen.generateFastPath(m_jit); > + > + JITCompiler::JumpList slowCases; > + slowCases.append(gen.slowPathJump()); I think we want to flushRegisters here since the most likely use of this is for set/get. Then, you'd also want to do the thing where you clear base/value/this from the set of usedRegisters. > Source/JavaScriptCore/jit/JITInlineCacheGenerator.cpp:171 > + JSValueRegs valueRegs, JSValueRegs baseRegs, JSValueRegs thisValueRegs, GPRReg scratch, What's the reason for this scratch parameter? I don't see a need for it. If all we're doing is clearing something from the used register set, I think this should just fall out naturally. Is there a reason why this is needed? > Source/JavaScriptCore/jit/JITOperations.cpp:556 > +void JIT_OPERATION operationPutByIdWithThisNonStrict(ExecState* exec, StructureStubInfo* stubInfo, EncodedJSValue encodedValue, EncodedJSValue encodedBase, EncodedJSValue encodedThis, UniquedStringImpl* uid) Style Nit: I would add "generic" to this name to match the above function for consistency. > Source/JavaScriptCore/jit/JITOperations.cpp:625 > + if (accessType != static_cast<AccessType>(stubInfo->accessType)) > + return; I know that the normal put IC path does this, but I don't understand how this could ever be true? I did a quick search, and don't see that this is assigned to after StructureStubInfo's constructor. Am I missing something? If I'm not missing anything, and this field never gets assigned to besides in the constructor, perhaps it's worth a cleanup either before or after your patch. > Source/JavaScriptCore/jit/JITPropertyAccess.cpp:753 > + // In order to be able to patch both the Structure, and the object offset, we store one pointer, > + // to just after the arguments have been loaded into registers 'hotPathBegin', and we generate code > + // such that the Structure & offset are always at the same distance from this. Why is this comment relevant? > Source/JavaScriptCore/jit/Repatch.cpp:372 > +#if USE(JSVALUE64) If you're doing this type of thing I'd also make the enumeration value exist on 64-bit platforms. > Source/JavaScriptCore/jit/Repatch.cpp:491 > + } else if (slot.base() != baseValue && slot.isCacheablePut() && putKind == WithThis) { Why do we only do this for WithThis? This seems wrong. > Source/JavaScriptCore/jit/Repatch.cpp:493 > + if (structure->isValidOffset(slot.cachedOffset())) { This seems wrong. Structure is for the base object, not the slot.base(), right? > Source/JavaScriptCore/jit/Repatch.cpp:498 > + if (stubInfo.cacheType == CacheType::Unset > + && InlineAccess::canGenerateSelfPropertyReplace(stubInfo, slot.cachedOffset()) > + && !structure->needImpurePropertyWatchpoint() > + && !structure->inferredTypeFor(ident.impl())) { This looks wrong, your above check for `slot.base() != baseValue` means this was not a self property replace.