Bug 162125 – Enable IC for put_by_id_with_this

• Component: JavaScriptCore
• Status: NEW

[Caio Lima]

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.

[Caio Lima]

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.

[Build Bot]

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.

[Build Bot]

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

[Build Bot]

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

[Caio Lima]

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.

[Build Bot]

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.

[Caio Lima]

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.

[Caio Lima]

Created attachment 305541 [details]
Patch

[Build Bot]

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.

[Caio Lima]

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.

[Build Bot]

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.

[Caio Lima]

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

[Caio Lima]

Ping review

[Saam Barati]

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.

[Caio Lima]

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.

[Caio Lima]

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.

[Build Bot]

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.

[Caio Lima]

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

[Build Bot]

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.

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Caio Lima]

Created attachment 308242 [details]
Proposed Patch

Fixing some NITs, rebased with upstream code and re-triggering EWS.

[Build Bot]

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.

[Saam Barati]

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.

[Caio Lima]

Created attachment 308330 [details]
Proposed Patch

Addressing Saam's review

[Build Bot]

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.

[Caio Lima]

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.

[Caio Lima]

Created attachment 308823 [details]
proposed Patch

Rebasing with master and ping for review.

[Build Bot]

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.

[Saam Barati]

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.