Use "this" instead of "callee" to get the constructor

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Ryosuke Niwa]

In order to support ES6 class syntax (bug 140491), [[Construct]] needs to take an extra argument newTarget.
Use "this" register to pass this argument since "this" is never used before the object allocation takes place.

[Ryosuke Niwa]

Created attachment 245593 [details]
Work in progress

[WebKit Commit Bot]

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


ERROR: Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:1130:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 1 in 11 files


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

[Oliver Hunt]

Comment on attachment 245593 [details]
Work in progress

all the disappearing code makes me happy

[Gavin Barraclough]

As well as de-duplicating the arguments to op_create_this, you can probably also remove op_get_callee, too!

[Ryosuke Niwa]

Geoff tells me he'd prefer keeping the two-arguments model so that the rest of the code base doens't need to know this quirk.  Also, we can't quite get rid of op_get_callee because my subsequent patches to implement ES6 class needs to use op_get_callee to get various private flags stored in the constructor.

[Ryosuke Niwa]

Created attachment 247180 [details]
Patch

[Filip Pizlo]

Comment on attachment 247180 [details]
Patch

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

> Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:-3884
>          arguments.append(jsCallee);
>          if (jsArguments)
>              arguments.append(jsArguments);
> -        if (thisArg)

Just above here there is the line:

arguments.append(m_out.constInt32(1 + !!jsArguments + !!thisArg));

I guess this can now be 2 + !!jsArguments

[Ryosuke Niwa]

Created attachment 247273 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 247273 [details]
Patch for landing

Clearing flags on attachment: 247273

Committed r180595: <http://trac.webkit.org/changeset/180595>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Benjamin Poulain]

Looks like this could be a 1.7% progression on Sunspider: http://arewefastyet.com/#machine=29

[Michael Saboff]

It looks like this caused test failures on iOS Arm 64

Test                                                                           r180594 r180600
regress/script-tests/deltablue-varargs.js.ftl-eager                             Passed  Failed
regress/script-tests/deltablue-varargs.js.ftl-no-cjit-no-inline-validate        Passed  Failed
stress/construct-varargs-inline-smaller-Foo.js.ftl-no-cjit-no-inline-validate   Passed  Failed
stress/construct-varargs-inline.js.ftl-eager                                    Passed  Failed
stress/construct-varargs-inline.js.ftl-eager-no-cjit                            Passed  Failed
stress/construct-varargs-inline.js.ftl-no-cjit-no-inline-validate               Passed  Failed
stress/construct-varargs-inline.js.ftl-no-cjit-validate                         Passed  Failed
stress/construct-varargs-no-inline.js.ftl-eager                                 Passed  Failed
stress/construct-varargs-no-inline.js.ftl-eager-no-cjit                         Passed  Failed
stress/construct-varargs-no-inline.js.ftl-no-cjit-no-inline-validate            Passed  Failed
stress/construct-varargs-no-inline.js.ftl-no-cjit-validate                      Passed  Failed

The failures have a similar output to:
[2015-02-25 00:40:02] INFO: stress/construct-varargs-inline.js.ftl-eager-no-cjit: Failed to insert inline cache for varargs call (specifically, ConstructVarargs) because we thought the size would be 284 but it ended up being 300 prior to compaction.
[2015-02-25 00:40:02] INFO: stress/construct-varargs-inline.js.ftl-eager-no-cjit: 1   0x1002af98c JSC::FTL::compile(JSC::FTL::State&, JSC::DFG::Safepoint::Result&)
[2015-02-25 00:40:02] INFO: stress/construct-varargs-inline.js.ftl-eager-no-cjit: 2   0x100226668 JSC::DFG::Plan::compileInThreadImpl(JSC::DFG::LongLivedState&)
[2015-02-25 00:40:02] INFO: stress/construct-varargs-inline.js.ftl-eager-no-cjit: 3   0x100225db0 JSC::DFG::Plan::compileInThread(JSC::DFG::LongLivedState&, JSC::DFG::ThreadData*)
[2015-02-25 00:40:02] INFO: stress/construct-varargs-inline.js.ftl-eager-no-cjit: 4   0x1001d15b0 JSC::DFG::compile(JSC::VM&, JSC::CodeBlock*, JSC::CodeBlock*, JSC::DFG::CompilationMode, unsigned int, JSC::Operands<JSC::JSValue, JSC::OperandValueTraits<JSC::JSValue> > const&, WTF::PassRefPtr<JSC::DeferredCompilationCallback>)
[2015-02-25 00:40:02] INFO: stress/construct-varargs-inline.js.ftl-eager-no-cjit: 5   0x10020a684 JSC::DFG::triggerFTLReplacementCompile(JSC::VM*, JSC::CodeBlock*, JSC::DFG::JITCode*)
[2015-02-25 00:40:02] INFO: stress/construct-varargs-inline.js.ftl-eager-no-cjit: 6   0x10020a37c triggerTierUpNow
[2015-02-25 00:40:02] INFO: stress/construct-varargs-inline.js.ftl-eager-no-cjit: 7   0x130adcf34
[2015-02-25 00:40:02] INFO: stress/construct-varargs-inline.js.ftl-eager-no-cjit: 8   0x12fae2938
[2015-02-25 00:40:02] INFO: stress/construct-varargs-inline.js.ftl-eager-no-cjit: 9   0x12fae3a7c
[2015-02-25 00:40:02] INFO: stress/construct-varargs-inline.js.ftl-eager-no-cjit: 10  0x100467848 vmEntryToJavaScript
[2015-02-25 00:40:02] INFO: stress/construct-varargs-inline.js.ftl-eager-no-cjit: 11  0x10037db84 JSC::JITCode::execute(JSC::VM*, JSC::ProtoCallFrame*)
[2015-02-25 00:40:02] INFO: stress/construct-varargs-inline.js.ftl-eager-no-cjit: 12  0x10036c508 JSC::Interpreter::execute(JSC::ProgramExecutable*, JSC::ExecState*, JSC::JSObject*)
[2015-02-25 00:40:02] INFO: stress/construct-varargs-inline.js.ftl-eager-no-cjit: 13  0x100159fd8 JSC::evaluate(JSC::ExecState*, JSC::SourceCode const&, JSC::JSValue, JSC::JSValue*)
[2015-02-25 00:40:02] INFO: stress/construct-varargs-inline.js.ftl-eager-no-cjit: 14  0x1000c9c60 jscmain(int, char**)
[2015-02-25 00:40:02] INFO: stress/construct-varargs-inline.js.ftl-eager-no-cjit: 15  0x1000c9670 main
[2015-02-25 00:40:02] INFO: stress/construct-varargs-inline.js.ftl-eager-no-cjit: 16  0x1980269e8 <redacted>
[2015-02-25 00:40:02] INFO: stress/construct-varargs-inline.js.ftl-eager-no-cjit: ./test_script_13755: line 2: 34513 Segmentation fault: 11  "$@" /System/Library/Frameworks/JavaScriptCore.framework/Resources/jsc --useFTLJIT\=false --enableFunctionDotArguments\=true --validateGraph\=true --useFTLJIT\=true --enableConcurrentJIT\=false --thresholdForJITAfterWarmUp\=100 --thresholdForJITAfterWarmUp\=10 --thresholdForJITSoon\=10 --thresholdForOptimizeAfterWarmUp\=20 --thresholdForOptimizeAfterLongWarmUp\=20 --thresholdForOptimizeSoon\=20 --thresholdForFTLOptimizeAfterWarmUp\=20 --thresholdForFTLOptimizeSoon\=20 --maximumEvalCacheableSourceLength\=150000 construct-varargs-inline.js
[2015-02-25 00:40:02] INFO: stress/construct-varargs-inline.js.ftl-eager-no-cjit: ERROR: Unexpected exit code: 139
[2015-02-25 00:40:02] ERROR: FAIL: stress/construct-varargs-inline.js.ftl-eager-no-cjit

[Ryosuke Niwa]

The regression was fixed in https://bugs.webkit.org/show_bug.cgi?id=142030.