Bug 141019 - Use "this" instead of "callee" to get the constructor
Summary: Use "this" instead of "callee" to get the constructor
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ryosuke Niwa
URL:
Keywords:
Depends on:
Blocks: 140491
  Show dependency treegraph
 
Reported: 2015-01-28 17:57 PST by Ryosuke Niwa
Modified: 2015-02-25 18:47 PST (History)
8 users (show)

See Also:


Attachments
Work in progress (16.16 KB, patch)
2015-01-28 18:00 PST, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Patch (31.26 KB, patch)
2015-02-23 18:13 PST, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Patch for landing (31.56 KB, patch)
2015-02-24 15:50 PST, Ryosuke Niwa
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryosuke Niwa 2015-01-28 17:57:16 PST
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.
Comment 1 Ryosuke Niwa 2015-01-28 18:00:54 PST
Created attachment 245593 [details]
Work in progress
Comment 2 WebKit Commit Bot 2015-01-28 18:13:42 PST
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.
Comment 3 Oliver Hunt 2015-01-28 18:33:04 PST
Comment on attachment 245593 [details]
Work in progress

all the disappearing code makes me happy
Comment 4 Gavin Barraclough 2015-01-28 20:23:52 PST
As well as de-duplicating the arguments to op_create_this, you can probably also remove op_get_callee, too!
Comment 5 Ryosuke Niwa 2015-02-23 18:12:49 PST
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.
Comment 6 Ryosuke Niwa 2015-02-23 18:13:02 PST
Created attachment 247180 [details]
Patch
Comment 7 Filip Pizlo 2015-02-24 14:28:38 PST
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
Comment 8 Ryosuke Niwa 2015-02-24 15:50:26 PST
Created attachment 247273 [details]
Patch for landing
Comment 9 WebKit Commit Bot 2015-02-24 16:41:53 PST
Comment on attachment 247273 [details]
Patch for landing

Clearing flags on attachment: 247273

Committed r180595: <http://trac.webkit.org/changeset/180595>
Comment 10 WebKit Commit Bot 2015-02-24 16:41:57 PST
All reviewed patches have been landed.  Closing bug.
Comment 11 Benjamin Poulain 2015-02-25 00:23:18 PST
Looks like this could be a 1.7% progression on Sunspider: http://arewefastyet.com/#machine=29
Comment 12 Michael Saboff 2015-02-25 10:25:42 PST
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
Comment 13 Ryosuke Niwa 2015-02-25 18:47:21 PST
The regression was fixed in https://bugs.webkit.org/show_bug.cgi?id=142030.