fourthTier: Re-worked non-local variable resolution
https://bugs.webkit.org/show_bug.cgi?id=117375
Reviewed by Filip Pizlo.
Source/JavaScriptCore:
This patch has two goals:
(1) Simplicity.
- Net removes 15 opcodes.
- Net removes 2,000 lines of code.
- Removes setPair() from the DFG: All DFG nodes have 1 result register now.
(2) Performance.
- 2%-3% speedup on SunSpider (20% in LLInt and Baseline JIT)
- 2% speedup on v8-spider
- 10% speedup on js-regress-hashmap*
- Amusing 2X speedup on js-regress-poly-stricteq
The bytecode now separates the scope chain resolution opcode from the
scope access opcode.
OLD:
get_scoped_var r0, 1, 0
inc r0
put_scoped_var 1, 0, r0
NEW:
resolve_scope r0, x(@id0)
get_from_scope r1, r0, x(@id0)
inc r1
put_to_scope r0, x(@id0), r1
Also, we link non-local variable resolution opcodes at CodeBlock link
time instead of time of first opcode execution.
This means that we can represent all possible non-local variable
resolutions using just three opcodes, and any optimizations in these
opcodes naturally apply across-the-board.
- API/JSCTestRunnerUtils.cpp:
(JSC::numberOfDFGCompiles):
(JSC::CodeBlock::dumpBytecode): Updated for removed things.
(JSC::CodeBlock::CodeBlock): Always provide the full scope chain when
creating a CodeBlock, so we can perform non-local variable resolution.
Added code to perform linking for these opcodes. This is where we figure
out which non-local variable resolutions are optimizable, and how.
(JSC::CodeBlock::finalizeUnconditionally):
(JSC::CodeBlock::noticeIncomingCall):
(JSC::CodeBlock::optimizeAfterWarmUp):
(JSC::CodeBlock::optimizeAfterLongWarmUp):
(JSC::CodeBlock::optimizeSoon): Updated for removed things.
(JSC::CodeBlock::needsActivation):
(JSC::GlobalCodeBlock::GlobalCodeBlock):
(JSC::ProgramCodeBlock::ProgramCodeBlock):
(JSC::EvalCodeBlock::EvalCodeBlock):
(JSC::FunctionCodeBlock::FunctionCodeBlock):
- bytecode/EvalCodeCache.h:
(JSC::EvalCodeCache::getSlow): Updated for interface changes.
- bytecode/GetByIdStatus.cpp:
(JSC::GetByIdStatus::computeFor): Treat global object access as
optimizable even though the global object has a custom property access
callback. This is what we've always done since, otherwise, we can't
optimize globals. (In future, we probably want to figure out a more
targeted policy than "any property access callback means no
optimization".)
- bytecode/GlobalResolveInfo.h: Removed.
- bytecode/Instruction.h:
- bytecode/Opcode.h:
(JSC::padOpcodeName):
- bytecode/PutByIdStatus.cpp:
(JSC::PutByIdStatus::computeFor): Like GetByIdStatus.
- bytecode/ResolveGlobalStatus.cpp: Removed.
- bytecode/ResolveGlobalStatus.h: Removed.
- bytecode/ResolveOperation.h: Removed.
- bytecode/UnlinkedCodeBlock.cpp:
(JSC::generateFunctionCodeBlock):
(JSC::UnlinkedFunctionExecutable::codeBlockFor):
(JSC::UnlinkedCodeBlock::UnlinkedCodeBlock):
- bytecode/UnlinkedCodeBlock.h: Don't provide a scope chain to unlinked
code blocks. Giving a scope to an unscoped compilation unit invites
programming errors.
(JSC::WatchpointSet::addressOfIsInvalidated):
- bytecompiler/BytecodeGenerator.cpp:
(JSC::BytecodeGenerator::BytecodeGenerator):
(JSC::BytecodeGenerator::resolveCallee):
(JSC::BytecodeGenerator::local):
(JSC::BytecodeGenerator::constLocal):
(JSC::BytecodeGenerator::resolveType):
(JSC::BytecodeGenerator::emitResolveScope):
(JSC::BytecodeGenerator::emitGetFromScope):
(JSC::BytecodeGenerator::emitPutToScope):
(JSC::BytecodeGenerator::emitInstanceOf):
(JSC::BytecodeGenerator::emitPushWithScope):
(JSC::BytecodeGenerator::emitPopScope):
(JSC::BytecodeGenerator::pushFinallyContext):
(JSC::BytecodeGenerator::emitComplexPopScopes):
(JSC::BytecodeGenerator::popTryAndEmitCatch):
(JSC::BytecodeGenerator::emitPushNameScope):
(JSC::BytecodeGenerator::isArgumentNumber):
- bytecompiler/BytecodeGenerator.h:
(JSC::Local::Local):
(JSC::Local::operator bool):
(JSC::Local::get):
(JSC::Local::isReadOnly):
(JSC::BytecodeGenerator::scopeDepth):
(JSC::BytecodeGenerator::shouldOptimizeLocals):
(JSC::BytecodeGenerator::canOptimizeNonLocals): Refactored the bytecode
generator to resolve all variables within local scope, as if there
were no non-local scope. This helps provide a separation of concerns:
unlinked bytecode is always scope-free, and the linking stage links
in the provided scope.
- bytecompiler/NodesCodegen.cpp:
(JSC::ResolveNode::isPure):
(JSC::ResolveNode::emitBytecode):
(JSC::EvalFunctionCallNode::emitBytecode):
(JSC::FunctionCallResolveNode::emitBytecode):
(JSC::PostfixNode::emitResolve):
(JSC::DeleteResolveNode::emitBytecode):
(JSC::TypeOfResolveNode::emitBytecode):
(JSC::PrefixNode::emitResolve):
(JSC::ReadModifyResolveNode::emitBytecode):
(JSC::AssignResolveNode::emitBytecode):
(JSC::ConstDeclNode::emitCodeSingle):
(JSC::ForInNode::emitBytecode): A bunch of this codegen is no longer
necessary, since it's redundant with the linking stage.
- dfg/DFGAbstractState.cpp:
(JSC::DFG::AbstractState::executeEffects):
- dfg/DFGByteCodeParser.cpp:
(JSC::DFG::ByteCodeParser::ByteCodeParser):
(JSC::DFG::ByteCodeParser::cellConstantWithStructureCheck):
(JSC::DFG::ByteCodeParser::handlePutByOffset):
(JSC::DFG::ByteCodeParser::handleGetById):
(JSC::DFG::ByteCodeParser::parseBlock): Updated for interface changes.
Notably, we can reuse existing DFG nodes -- but the mapping between
bytecode and DFG nodes has changed, and some nodes and corner cases have
been removed.
(JSC::DFG::CSEPhase::scopedVarLoadElimination):
(JSC::DFG::CSEPhase::varInjectionWatchpointElimination):
(JSC::DFG::CSEPhase::globalVarStoreElimination):
(JSC::DFG::CSEPhase::scopedVarStoreElimination):
(JSC::DFG::CSEPhase::getLocalLoadElimination):
(JSC::DFG::CSEPhase::setLocalStoreElimination):
(JSC::DFG::CSEPhase::performNodeCSE): Added CSE for var injection
watchpoints. Even though watchpoints are "free", they're quite common
inside code that's subject to var injection, so I figured we'd save a
little memory.
(JSC::DFG::capabilityLevel):
- dfg/DFGCapabilities.h: Removed detection for old forms.
(JSC::DFG::tryCompile):
(JSC::DFG::tryCompileFunction):
(JSC::DFG::FixupPhase::fixupNode):
- dfg/DFGGraph.h:
- dfg/DFGJITCode.cpp:
- dfg/DFGNode.h:
(JSC::DFG::Node::convertToStructureTransitionWatchpoint):
(JSC::DFG::Node::hasVarNumber):
(JSC::DFG::Node::hasIdentifierNumberForCheck):
(JSC::DFG::Node::hasRegisterPointer):
(JSC::DFG::Node::hasHeapPrediction):
- dfg/DFGNodeType.h:
- dfg/DFGOperations.cpp:
- dfg/DFGOperations.h:
- dfg/DFGPredictionPropagationPhase.cpp:
(JSC::DFG::PredictionPropagationPhase::propagate):
(JSC::DFG::dfgResetGetByID):
(JSC::DFG::dfgResetPutByID):
(JSC::DFG::SpeculativeJIT::callOperation): Removed some unneeded things,
and updated for renames.
- dfg/DFGSpeculativeJIT32_64.cpp:
(JSC::DFG::SpeculativeJIT::compile):
- dfg/DFGSpeculativeJIT64.cpp:
(JSC::DFG::SpeculativeJIT::compile): The two primary changes here are:
(1) Use a watchpoint for var injection instead of looping over the scope
chain and checking. This is more efficient and much easier to model in
code generation.
(2) I've eliminated the notion of an optimized global assignment that
needs to check for whether it should fire a watchpiont. Instead, we
fire pre-emptively at the point of optimization. This removes a bunch
of edge cases, and it seems like a more honest representation of
the fact that our new optimization contradicts our old one.
- dfg/DFGTypeCheckHoistingPhase.cpp:
(JSC::DFG::TypeCheckHoistingPhase::identifyRedundantStructureChecks):
(JSC::DFG::TypeCheckHoistingPhase::identifyRedundantArrayChecks):
(JSC::DFGCodeBlocks::jettison):
(JSC::ExecState::trueCallFrame): Removed stuff that's unused now, and
fixed the build.
- interpreter/Interpreter.cpp:
(JSC::eval):
(JSC::getBytecodeOffsetForCallFrame):
(JSC::getCallerInfo):
(JSC::Interpreter::throwException): Updated exception scope tracking
to match the rest of our linking strategy: The unlinked bytecode compiles
exception scope as if non-local scope did not exist, and we add in
non-local scope at link time. This means that we can restore the right
scope depth based on a simple number, without checking the contents of
the scope chain.
(JSC::Interpreter::execute): Make sure to establish the full scope chain
before linking eval code. We now require the full scope chain at link
time, in order to link non-local variable resolution opcodes.
(JSC::JIT::JIT):
(JSC::JIT::privateCompileMainPass):
(JSC::JIT::privateCompileSlowCases):
- jit/JIT.h:
- jit/JITArithmetic.cpp:
(JSC::JIT::emit_op_add):
- jit/JITCode.cpp:
- jit/JITOpcodes.cpp:
(JSC::JIT::emitSlow_op_bitxor):
(JSC::JIT::emitSlow_op_bitor):
(JSC::JIT::emitSlow_op_to_primitive):
(JSC::JIT::emit_op_strcat):
(JSC::JIT::emitSlow_op_create_this):
(JSC::JIT::emitSlow_op_to_this):
- jit/JITPropertyAccess.cpp:
(JSC::JIT::emitVarInjectionCheck):
(JSC::JIT::emitResolveClosure):
(JSC::JIT::emit_op_resolve_scope):
(JSC::JIT::emitSlow_op_resolve_scope):
(JSC::JIT::emitLoadWithStructureCheck):
(JSC::JIT::emitGetGlobalProperty):
(JSC::JIT::emitGetGlobalVar):
(JSC::JIT::emitGetClosureVar):
(JSC::JIT::emit_op_get_from_scope):
(JSC::JIT::emitSlow_op_get_from_scope):
(JSC::JIT::emitPutGlobalProperty):
(JSC::JIT::emitPutGlobalVar):
(JSC::JIT::emitPutClosureVar):
(JSC::JIT::emit_op_put_to_scope):
(JSC::JIT::emitSlow_op_put_to_scope):
(JSC::JIT::emit_op_init_global_const):
- jit/JITPropertyAccess32_64.cpp:
(JSC::JIT::emitVarInjectionCheck):
(JSC::JIT::emitResolveClosure):
(JSC::JIT::emit_op_resolve_scope):
(JSC::JIT::emitSlow_op_resolve_scope):
(JSC::JIT::emitLoadWithStructureCheck):
(JSC::JIT::emitGetGlobalProperty):
(JSC::JIT::emitGetGlobalVar):
(JSC::JIT::emitGetClosureVar):
(JSC::JIT::emit_op_get_from_scope):
(JSC::JIT::emitSlow_op_get_from_scope):
(JSC::JIT::emitPutGlobalProperty):
(JSC::JIT::emitPutGlobalVar):
(JSC::JIT::emitPutClosureVar):
(JSC::JIT::emit_op_put_to_scope):
(JSC::JIT::emitSlow_op_put_to_scope):
(JSC::JIT::emit_op_init_global_const):
(JSC::DEFINE_STUB_FUNCTION):
- jit/JITStubs.h: Re-wrote baseline JIT codegen for our new variable
resolution model.
(JSC::LLInt::Data::performAssertions):
- llint/LLIntSlowPaths.cpp:
- llint/LLIntSlowPaths.h:
- llint/LowLevelInterpreter.asm:
- llint/LowLevelInterpreter.cpp:
(JSC::CLoop::execute):
- llint/LowLevelInterpreter32_64.asm:
- llint/LowLevelInterpreter64.asm: Ditto for LLInt.
- offlineasm/x86.rb: Fixed a pre-existing encoding bug for a syntactic
form that we never used before.
- runtime/ArrayPrototype.cpp:
(JSC::arrayProtoFuncToString):
(JSC::arrayProtoFuncToLocaleString):
(JSC::arrayProtoFuncJoin):
(JSC::arrayProtoFuncConcat):
(JSC::arrayProtoFuncPop):
(JSC::arrayProtoFuncPush):
(JSC::arrayProtoFuncReverse):
(JSC::arrayProtoFuncShift):
(JSC::arrayProtoFuncSlice):
(JSC::arrayProtoFuncSort):
(JSC::arrayProtoFuncSplice):
(JSC::arrayProtoFuncUnShift):
(JSC::arrayProtoFuncFilter):
(JSC::arrayProtoFuncMap):
(JSC::arrayProtoFuncEvery):
(JSC::arrayProtoFuncForEach):
(JSC::arrayProtoFuncSome):
(JSC::arrayProtoFuncReduce):
(JSC::arrayProtoFuncReduceRight):
(JSC::arrayProtoFuncIndexOf):
(JSC::arrayProtoFuncLastIndexOf): Fixed some pre-existing bugs in
'this' value conversion, which I made much more common by removing
special cases in bytecode generation.
These functions need to invoke toThis() because they observe the 'this'
value. Also, toLocaleString() is specified to accept non-array 'this'
values.
(Most other host functions don't need this fix because they perform
strict 'this' checking, which never coerces unexpected types.)
(JSC::CodeCache::getCodeBlock):
(JSC::CodeCache::getProgramCodeBlock):
(JSC::CodeCache::getEvalCodeBlock):
- runtime/CodeCache.h: Don't supply a scope to the unlinked code cache.
Unlinked code is supposed to be scope-free, so let's have the compiler
help verify that.
- runtime/CommonSlowPaths.cpp:
(JSC::SLOW_PATH_DECL):
- runtime/CommonSlowPaths.h:
- runtime/Executable.cpp:
(JSC::EvalExecutable::create):
(JSC::EvalExecutable::compileInternal):
(JSC::ProgramExecutable::compileInternal):
(JSC::FunctionExecutable::produceCodeBlockFor):
(JSC::FunctionExecutable::compileForCallInternal):
(JSC::FunctionExecutable::compileForConstructInternal):
(JSC::EvalExecutable::numVariables):
(JSC::EvalExecutable::numberOfFunctionDecls):
- runtime/ExecutionHarness.h:
(JSC::prepareForExecutionImpl):
(JSC::prepareFunctionForExecutionImpl):
(JSC::installOptimizedCode): Fiddled with executable initialization so
that we can always generate a full scope chain before we go to link a
code block. We need this because code block linking now depends on the
scope chain to link non-local variable resolution opcodes.
- runtime/JSActivation.h:
- runtime/JSGlobalObject.cpp:
(JSC::JSGlobalObject::JSGlobalObject):
(JSC::JSGlobalObject::createEvalCodeBlock):
- runtime/JSGlobalObject.h:
(JSC::JSGlobalObject::varInjectionWatchpoint):
- runtime/JSGlobalObjectFunctions.cpp:
(JSC::globalFuncEval):
- runtime/JSNameScope.h:
- runtime/JSScope.cpp:
(JSC::abstractAccess):
(JSC::JSScope::objectAtScope):
(JSC::JSScope::depth):
(JSC::JSScope::resolve):
(JSC::JSScope::abstractResolve): Updated to match changes explained above.
(JSC::makeType):
(JSC::needsVarInjectionChecks):
(JSC::ResolveOp::ResolveOp):
(JSC::ResolveModeAndType::ResolveModeAndType):
(JSC::ResolveModeAndType::mode):
(JSC::ResolveModeAndType::type):
(JSC::ResolveModeAndType::operand): Removed the old variable resolution
state machine, since it's unused now. Added logic for performing abstract
variable resolution at link time. This is used by codeblock linking.
- runtime/ObjectPrototype.cpp:
(JSC::objectProtoFuncValueOf):
(JSC::objectProtoFuncHasOwnProperty):
(JSC::objectProtoFuncIsPrototypeOf):
(JSC::objectProtoFuncDefineGetter):
(JSC::objectProtoFuncDefineSetter):
(JSC::objectProtoFuncLookupGetter):
(JSC::objectProtoFuncLookupSetter):
(JSC::objectProtoFuncPropertyIsEnumerable):
(JSC::objectProtoFuncToLocaleString):
(JSC::objectProtoFuncToString): Fixed some pre-existing bugs in
'this' value conversion, which I made much more common by removing
special cases in bytecode generation.
These functions need to invoke toThis() because they observe the 'this'
value.
- runtime/StringPrototype.cpp:
(JSC::checkObjectCoercible):
(JSC::stringProtoFuncReplace):
(JSC::stringProtoFuncCharAt):
(JSC::stringProtoFuncCharCodeAt):
(JSC::stringProtoFuncConcat):
(JSC::stringProtoFuncIndexOf):
(JSC::stringProtoFuncLastIndexOf):
(JSC::stringProtoFuncMatch):
(JSC::stringProtoFuncSearch):
(JSC::stringProtoFuncSlice):
(JSC::stringProtoFuncSplit):
(JSC::stringProtoFuncSubstr):
(JSC::stringProtoFuncSubstring):
(JSC::stringProtoFuncToLowerCase):
(JSC::stringProtoFuncToUpperCase):
(JSC::stringProtoFuncLocaleCompare):
(JSC::stringProtoFuncBig):
(JSC::stringProtoFuncSmall):
(JSC::stringProtoFuncBlink):
(JSC::stringProtoFuncBold):
(JSC::stringProtoFuncFixed):
(JSC::stringProtoFuncItalics):
(JSC::stringProtoFuncStrike):
(JSC::stringProtoFuncSub):
(JSC::stringProtoFuncSup):
(JSC::stringProtoFuncFontcolor):
(JSC::stringProtoFuncFontsize):
(JSC::stringProtoFuncAnchor):
(JSC::stringProtoFuncLink):
(JSC::trimString): Fixed some pre-existing bugs in
'this' value conversion, which I made much more common by removing
special cases in bytecode generation.
These functions need to invoke toThis() because they observe the 'this'
value.
- runtime/StructureRareData.cpp:
- runtime/VM.cpp:
(JSC::VM::~VM):
(JSC::WriteBarrierBase::slot): Modified to reduce casting in client code.
LayoutTests:
This patch removed special-case 'this' resolution from bytecode, making
some pre-existing edge cases in 'this' value treatment much more common.
I updated the test results below, and added some tests, to match bug
fixes for these cases.
- fast/js/script-tests/array-functions-non-arrays.js:
- fast/js/array-functions-non-arrays-expected.txt: As specified, it's
not an error to pass a non-array to toLocaleString. Our new result
matches Firefox and Chrome.
- fast/js/array-prototype-properties-expected.txt: Updated for slightly
clearer error message.
- fast/js/basic-strict-mode-expected.txt: Updated for slightly more
standard error message.
- fast/js/object-prototype-toString-expected.txt: Added.
- fast/js/object-prototype-toString.html: Added. This test demonstrates
why we now fail a Sputnik test below, while Firefox and Chrome pass it.
(The test doesn't test what it thinks it tests, and this test verifies
that we get right what it does think it tests.)
- fast/js/string-prototype-function-this-expected.txt: Added.
- fast/js/string-prototype-function-this.html: Added. This test shows
that we CheckObjectCoercible in string prototype functions. (We used
to get this wrong, but Sputnik tests made it seem like we got it right
because they didn't test the dynamic scope case.)
- sputnik/Conformance/11_Expressions/11.1_Primary_Expressions/11.1.1_The_this_Keyword/S11.1.1_A2-expected.txt:
- sputnik/Conformance/15_Native_Objects/15.4_Array/15.4.4/15.4.4.3_Array_prototype_toLocaleString/S15.4.4.3_A2_T1-expected.txt:
- sputnik/Conformance/15_Native_Objects/15.5_String/15.5.4/15.5.4.10_String.prototype.match/S15.5.4.10_A1_T3-expected.txt:
- sputnik/Conformance/15_Native_Objects/15.5_String/15.5.4/15.5.4.11_String.prototype.replace/S15.5.4.11_A1_T3-expected.txt:
- sputnik/Conformance/15_Native_Objects/15.5_String/15.5.4/15.5.4.12_String.prototype.search/S15.5.4.12_A1_T3-expected.txt:
- sputnik/Conformance/15_Native_Objects/15.5_String/15.5.4/15.5.4.13_String.prototype.slice/S15.5.4.13_A1_T3-expected.txt:
- sputnik/Conformance/15_Native_Objects/15.5_String/15.5.4/15.5.4.14_String.prototype.split/S15.5.4.14_A1_T3-expected.txt:
- sputnik/Conformance/15_Native_Objects/15.5_String/15.5.4/15.5.4.15_String.prototype.substring/S15.5.4.15_A1_T3-expected.txt:
- sputnik/Conformance/15_Native_Objects/15.5_String/15.5.4/15.5.4.6_String.prototype.concat/S15.5.4.6_A1_T3-expected.txt:
- sputnik/Conformance/15_Native_Objects/15.5_String/15.5.4/15.5.4.7_String.prototype.indexOf/S15.5.4.7_A1_T3-expected.txt:
- sputnik/Conformance/15_Native_Objects/15.5_String/15.5.4/15.5.4.8_String.prototype.lastIndexOf/S15.5.4.8_A1_T3-expected.txt:
Updated to show failing results. Firefox and Chrome also fail these
tests, and the ES5 spec seems to mandate failure. Because these tests
resolve a String.prototype function at global scope, the 'this' value
for the call is an environment record. Logically, an environment record
converts to 'undefined' at the call site, and should then fail the
CheckObjectCoercible test.
