Skip to content

Commit

Permalink
[JSC] Do not declare callee name when it is function declaration
Browse files Browse the repository at this point in the history
https://bugs.webkit.org/show_bug.cgi?id=264030
rdar://117786144

Reviewed by Alexey Shvayka.

We should declare callee only when it is not function declarations. Otherwise, we taint the scope with that,
and unnecessarily create a JSLexicalEnvironment when it gets captured.

    function test(n) {
        if (n) {
            (function(n) {
                test(n - 1);
            }(n));
        }
    }

In the above code, if `test` function is function declaration, we should define `test` name in the upper scope
and we do not need to declare it inside the function.
We propagate FunctionMode in Parser and use it as the same way to BytecodeGenerator.

Microbenchmark shows improvement since we wipe JSLexicalEnvironment allocations.

                                          ToT                     Patched

    function-declaration-name       32.8020+-0.3279     ^     27.4767+-0.2500        ^ definitely 1.1938x faster

* Source/JavaScriptCore/API/JSScriptRef.cpp:
(parseScript):
* Source/JavaScriptCore/API/glib/JSCContext.cpp:
(jsc_context_check_syntax):
* Source/JavaScriptCore/builtins/BuiltinExecutables.cpp:
(JSC::BuiltinExecutables::createExecutable):
* Source/JavaScriptCore/bytecode/UnlinkedFunctionExecutable.cpp:
(JSC::generateUnlinkedFunctionCodeBlock):
* Source/JavaScriptCore/debugger/DebuggerParseData.cpp:
(JSC::gatherDebuggerParseData):
* Source/JavaScriptCore/parser/Parser.cpp:
(JSC::Parser<LexerType>::Parser):
(JSC::Parser<LexerType>::parseInner):
* Source/JavaScriptCore/parser/Parser.h:
(JSC::Parser::functionMode const):
(JSC::parse):
(JSC::parseFunctionForFunctionConstructor):
* Source/JavaScriptCore/parser/ParserModes.h:
* Source/JavaScriptCore/runtime/CodeCache.cpp:
(JSC::generateUnlinkedCodeBlockImpl):
* Source/JavaScriptCore/runtime/Completion.cpp:
(JSC::checkSyntaxInternal):
(JSC::checkModuleSyntax):
* Source/JavaScriptCore/runtime/JSModuleLoader.cpp:
(JSC::JSC_DEFINE_HOST_FUNCTION):

Canonical link: https://commits.webkit.org/270095@main
  • Loading branch information
Constellation committed Nov 2, 2023
1 parent 2b7aa25 commit 3e0dc21
Show file tree
Hide file tree
Showing 12 changed files with 39 additions and 19 deletions.
17 changes: 17 additions & 0 deletions JSTests/microbenchmarks/function-declaration-name.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
function shouldBe(actual, expected) {
if (actual !== expected)
throw new Error('bad value: ' + actual);
}

(function () {
"use strict";
function test(n, count) {
if (n) {
return (function (n, count) {
return test(n, count);
}(n - 1, count + 1));
}
return count;
}
shouldBe(test(1e7, 0), 1e7);
}());
2 changes: 1 addition & 1 deletion Source/JavaScriptCore/API/JSScriptRef.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ static bool parseScript(VM& vm, const SourceCode& source, ParserError& error)
{
return !!JSC::parse<JSC::ProgramNode>(
vm, source, Identifier(), ImplementationVisibility::Public, JSParserBuiltinMode::NotBuiltin,
JSParserStrictMode::NotStrict, JSParserScriptMode::Classic, SourceParseMode::ProgramMode, SuperBinding::NotNeeded,
JSParserStrictMode::NotStrict, JSParserScriptMode::Classic, SourceParseMode::ProgramMode, FunctionMode::None, SuperBinding::NotNeeded,
error);
}

Expand Down
4 changes: 2 additions & 2 deletions Source/JavaScriptCore/API/glib/JSCContext.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -980,11 +980,11 @@ JSCCheckSyntaxResult jsc_context_check_syntax(JSCContext* context, const char* c
switch (mode) {
case JSC_CHECK_SYNTAX_MODE_SCRIPT:
success = !!JSC::parse<JSC::ProgramNode>(vm, source, JSC::Identifier(), JSC::ImplementationVisibility::Public, JSC::JSParserBuiltinMode::NotBuiltin,
JSC::JSParserStrictMode::NotStrict, JSC::JSParserScriptMode::Classic, JSC::SourceParseMode::ProgramMode, JSC::SuperBinding::NotNeeded, error);
JSC::JSParserStrictMode::NotStrict, JSC::JSParserScriptMode::Classic, JSC::SourceParseMode::ProgramMode, JSC::FunctionMode::None, JSC::SuperBinding::NotNeeded, error);
break;
case JSC_CHECK_SYNTAX_MODE_MODULE:
success = !!JSC::parse<JSC::ModuleProgramNode>(vm, source, JSC::Identifier(), JSC::ImplementationVisibility::Public, JSC::JSParserBuiltinMode::NotBuiltin,
JSC::JSParserStrictMode::Strict, JSC::JSParserScriptMode::Module, JSC::SourceParseMode::ModuleAnalyzeMode, JSC::SuperBinding::NotNeeded, error);
JSC::JSParserStrictMode::Strict, JSC::JSParserScriptMode::Module, JSC::SourceParseMode::ModuleAnalyzeMode, JSC::FunctionMode::None, JSC::SuperBinding::NotNeeded, error);
break;
}

Expand Down
2 changes: 1 addition & 1 deletion Source/JavaScriptCore/builtins/BuiltinExecutables.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -218,7 +218,7 @@ UnlinkedFunctionExecutable* BuiltinExecutables::createExecutable(VM& vm, const S
JSParserBuiltinMode builtinMode = isBuiltinDefaultClassConstructor ? JSParserBuiltinMode::NotBuiltin : JSParserBuiltinMode::Builtin;
std::unique_ptr<ProgramNode> program = parse<ProgramNode>(
vm, source, Identifier(), implementationVisibility, builtinMode,
JSParserStrictMode::NotStrict, JSParserScriptMode::Classic, SourceParseMode::ProgramMode, SuperBinding::NotNeeded, error,
JSParserStrictMode::NotStrict, JSParserScriptMode::Classic, SourceParseMode::ProgramMode, FunctionMode::None, SuperBinding::NotNeeded, error,
&positionBeforeLastNewlineFromParser, constructorKind);

if (program) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ static UnlinkedFunctionCodeBlock* generateUnlinkedFunctionCodeBlock(
ASSERT(isFunctionParseMode(executable->parseMode()));
auto* classFieldLocations = executable->classFieldLocations();
std::unique_ptr<FunctionNode> function = parse<FunctionNode>(
vm, source, executable->name(), executable->implementationVisibility(), builtinMode, strictMode, scriptMode, executable->parseMode(), executable->superBinding(), error, nullptr, ConstructorKind::None, DerivedContextType::None, EvalContextType::None, nullptr, nullptr, classFieldLocations);
vm, source, executable->name(), executable->implementationVisibility(), builtinMode, strictMode, scriptMode, executable->parseMode(), executable->functionMode(), executable->superBinding(), error, nullptr, ConstructorKind::None, DerivedContextType::None, EvalContextType::None, nullptr, nullptr, classFieldLocations);

if (!function) {
ASSERT(error.isValid());
Expand Down
2 changes: 1 addition & 1 deletion Source/JavaScriptCore/debugger/DebuggerParseData.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -177,7 +177,7 @@ bool gatherDebuggerParseData(VM& vm, const SourceCode& source, DebuggerParseData

ParserError error;
std::unique_ptr<RootNode> rootNode = parse<RootNode>(vm, source, Identifier(), ImplementationVisibility::Public,
JSParserBuiltinMode::NotBuiltin, strictMode, scriptMode, parseMode, SuperBinding::NotNeeded,
JSParserBuiltinMode::NotBuiltin, strictMode, scriptMode, parseMode, FunctionMode::None, SuperBinding::NotNeeded,
error, nullptr, ConstructorKind::None, DerivedContextType::None, EvalContextType::None,
&debuggerParseData);
if (!rootNode)
Expand Down
5 changes: 3 additions & 2 deletions Source/JavaScriptCore/parser/Parser.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,7 @@ void Parser<LexerType>::logError(bool shouldPrintToken, Args&&... args)
}

template <typename LexerType>
Parser<LexerType>::Parser(VM& vm, const SourceCode& source, ImplementationVisibility implementationVisibility, JSParserBuiltinMode builtinMode, JSParserStrictMode strictMode, JSParserScriptMode scriptMode, SourceParseMode parseMode, SuperBinding superBinding, ConstructorKind defaultConstructorKindForTopLevelFunction, DerivedContextType derivedContextType, bool isEvalContext, EvalContextType evalContextType, DebuggerParseData* debuggerParseData, bool isInsideOrdinaryFunction)
Parser<LexerType>::Parser(VM& vm, const SourceCode& source, ImplementationVisibility implementationVisibility, JSParserBuiltinMode builtinMode, JSParserStrictMode strictMode, JSParserScriptMode scriptMode, SourceParseMode parseMode, FunctionMode functionMode, SuperBinding superBinding, ConstructorKind defaultConstructorKindForTopLevelFunction, DerivedContextType derivedContextType, bool isEvalContext, EvalContextType evalContextType, DebuggerParseData* debuggerParseData, bool isInsideOrdinaryFunction)
: m_vm(vm)
, m_source(&source)
, m_hasStackOverflow(false)
Expand All @@ -136,6 +136,7 @@ Parser<LexerType>::Parser(VM& vm, const SourceCode& source, ImplementationVisibi
, m_implementationVisibility(implementationVisibility)
, m_parsingBuiltin(builtinMode == JSParserBuiltinMode::Builtin)
, m_parseMode(parseMode)
, m_functionMode(functionMode)
, m_scriptMode(scriptMode)
, m_superBinding(superBinding)
, m_defaultConstructorKindForTopLevelFunction(defaultConstructorKindForTopLevelFunction)
Expand Down Expand Up @@ -262,7 +263,7 @@ Expected<typename Parser<LexerType>::ParseInnerResult, String> Parser<LexerType>
}
}

if (!calleeName.isNull())
if (functionNameIsInScope(calleeName, functionMode()))
scope->declareCallee(&calleeName);

if (m_lexer->isReparsingFunction())
Expand Down
14 changes: 8 additions & 6 deletions Source/JavaScriptCore/parser/Parser.h
Original file line number Diff line number Diff line change
Expand Up @@ -1035,7 +1035,7 @@ class Parser {
WTF_MAKE_FAST_ALLOCATED;

public:
Parser(VM&, const SourceCode&, ImplementationVisibility, JSParserBuiltinMode, JSParserStrictMode, JSParserScriptMode, SourceParseMode, SuperBinding, ConstructorKind defaultConstructorKindForTopLevelFunction = ConstructorKind::None, DerivedContextType = DerivedContextType::None, bool isEvalContext = false, EvalContextType = EvalContextType::None, DebuggerParseData* = nullptr, bool isInsideOrdinaryFunction = false);
Parser(VM&, const SourceCode&, ImplementationVisibility, JSParserBuiltinMode, JSParserStrictMode, JSParserScriptMode, SourceParseMode, FunctionMode, SuperBinding, ConstructorKind defaultConstructorKindForTopLevelFunction = ConstructorKind::None, DerivedContextType = DerivedContextType::None, bool isEvalContext = false, EvalContextType = EvalContextType::None, DebuggerParseData* = nullptr, bool isInsideOrdinaryFunction = false);
~Parser();

template <class ParsedNode>
Expand Down Expand Up @@ -1265,6 +1265,7 @@ class Parser {
}

ALWAYS_INLINE SourceParseMode sourceParseMode() const { return m_parseMode; }
ALWAYS_INLINE FunctionMode functionMode() const { return m_functionMode; }
ALWAYS_INLINE bool isEvalOrArguments(const Identifier* ident) { return isEvalOrArgumentsIdentifier(m_vm, ident); }

ScopeRef upperScope(int n)
Expand Down Expand Up @@ -2160,6 +2161,7 @@ class Parser {
ImplementationVisibility m_implementationVisibility;
bool m_parsingBuiltin;
SourceParseMode m_parseMode;
FunctionMode m_functionMode;
JSParserScriptMode m_scriptMode;
SuperBinding m_superBinding;
ConstructorKind m_defaultConstructorKindForTopLevelFunction;
Expand Down Expand Up @@ -2272,7 +2274,7 @@ template <class ParsedNode>
std::unique_ptr<ParsedNode> parse(
VM& vm, const SourceCode& source,
const Identifier& name, ImplementationVisibility implementationVisibility, JSParserBuiltinMode builtinMode,
JSParserStrictMode strictMode, JSParserScriptMode scriptMode, SourceParseMode parseMode, SuperBinding superBinding,
JSParserStrictMode strictMode, JSParserScriptMode scriptMode, SourceParseMode parseMode, FunctionMode functionMode, SuperBinding superBinding,
ParserError& error, JSTextPosition* positionBeforeLastNewline = nullptr,
ConstructorKind defaultConstructorKindForTopLevelFunction = ConstructorKind::None,
DerivedContextType derivedContextType = DerivedContextType::None,
Expand All @@ -2290,7 +2292,7 @@ std::unique_ptr<ParsedNode> parse(

std::unique_ptr<ParsedNode> result;
if (source.provider()->source().is8Bit()) {
Parser<Lexer<LChar>> parser(vm, source, implementationVisibility, builtinMode, strictMode, scriptMode, parseMode, superBinding, defaultConstructorKindForTopLevelFunction, derivedContextType, isEvalNode<ParsedNode>(), evalContextType, debuggerParseData, isInsideOrdinaryFunction);
Parser<Lexer<LChar>> parser(vm, source, implementationVisibility, builtinMode, strictMode, scriptMode, parseMode, functionMode, superBinding, defaultConstructorKindForTopLevelFunction, derivedContextType, isEvalNode<ParsedNode>(), evalContextType, debuggerParseData, isInsideOrdinaryFunction);
result = parser.parse<ParsedNode>(error, name, isEvalNode<ParsedNode>() ? ParsingContext::Eval : ParsingContext::Program, std::nullopt, parentScopePrivateNames, classFieldLocations);
if (positionBeforeLastNewline)
*positionBeforeLastNewline = parser.positionBeforeLastNewline();
Expand All @@ -2303,7 +2305,7 @@ std::unique_ptr<ParsedNode> parse(
}
} else {
ASSERT_WITH_MESSAGE(defaultConstructorKindForTopLevelFunction == ConstructorKind::None, "BuiltinExecutables's special constructors should always use a 8-bit string");
Parser<Lexer<UChar>> parser(vm, source, implementationVisibility, builtinMode, strictMode, scriptMode, parseMode, superBinding, defaultConstructorKindForTopLevelFunction, derivedContextType, isEvalNode<ParsedNode>(), evalContextType, debuggerParseData, isInsideOrdinaryFunction);
Parser<Lexer<UChar>> parser(vm, source, implementationVisibility, builtinMode, strictMode, scriptMode, parseMode, functionMode, superBinding, defaultConstructorKindForTopLevelFunction, derivedContextType, isEvalNode<ParsedNode>(), evalContextType, debuggerParseData, isInsideOrdinaryFunction);
result = parser.parse<ParsedNode>(error, name, isEvalNode<ParsedNode>() ? ParsingContext::Eval : ParsingContext::Program, std::nullopt, parentScopePrivateNames, classFieldLocations);
if (positionBeforeLastNewline)
*positionBeforeLastNewline = parser.positionBeforeLastNewline();
Expand Down Expand Up @@ -2333,12 +2335,12 @@ inline std::unique_ptr<ProgramNode> parseFunctionForFunctionConstructor(VM& vm,
bool isEvalNode = false;
std::unique_ptr<ProgramNode> result;
if (source.provider()->source().is8Bit()) {
Parser<Lexer<LChar>> parser(vm, source, ImplementationVisibility::Public, JSParserBuiltinMode::NotBuiltin, JSParserStrictMode::NotStrict, JSParserScriptMode::Classic, SourceParseMode::ProgramMode, SuperBinding::NotNeeded, ConstructorKind::None, DerivedContextType::None, isEvalNode, EvalContextType::None, nullptr);
Parser<Lexer<LChar>> parser(vm, source, ImplementationVisibility::Public, JSParserBuiltinMode::NotBuiltin, JSParserStrictMode::NotStrict, JSParserScriptMode::Classic, SourceParseMode::ProgramMode, FunctionMode::None, SuperBinding::NotNeeded, ConstructorKind::None, DerivedContextType::None, isEvalNode, EvalContextType::None, nullptr);
result = parser.parse<ProgramNode>(error, name, ParsingContext::FunctionConstructor, functionConstructorParametersEndPosition);
if (positionBeforeLastNewline)
*positionBeforeLastNewline = parser.positionBeforeLastNewline();
} else {
Parser<Lexer<UChar>> parser(vm, source, ImplementationVisibility::Public, JSParserBuiltinMode::NotBuiltin, JSParserStrictMode::NotStrict, JSParserScriptMode::Classic, SourceParseMode::ProgramMode, SuperBinding::NotNeeded, ConstructorKind::None, DerivedContextType::None, isEvalNode, EvalContextType::None, nullptr);
Parser<Lexer<UChar>> parser(vm, source, ImplementationVisibility::Public, JSParserBuiltinMode::NotBuiltin, JSParserStrictMode::NotStrict, JSParserScriptMode::Classic, SourceParseMode::ProgramMode, FunctionMode::None, SuperBinding::NotNeeded, ConstructorKind::None, DerivedContextType::None, isEvalNode, EvalContextType::None, nullptr);
result = parser.parse<ProgramNode>(error, name, ParsingContext::FunctionConstructor, functionConstructorParametersEndPosition);
if (positionBeforeLastNewline)
*positionBeforeLastNewline = parser.positionBeforeLastNewline();
Expand Down
2 changes: 1 addition & 1 deletion Source/JavaScriptCore/parser/ParserModes.h
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ enum class CodeGenerationMode : uint8_t {
ControlFlowProfiler = 1 << 2,
};

enum class FunctionMode { FunctionExpression, FunctionDeclaration, MethodDefinition };
enum class FunctionMode { None, FunctionExpression, FunctionDeclaration, MethodDefinition };

// Keep it less than 32, it means this should be within 5 bits.
enum class SourceParseMode : uint8_t {
Expand Down
2 changes: 1 addition & 1 deletion Source/JavaScriptCore/runtime/CodeCache.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ UnlinkedCodeBlockType* generateUnlinkedCodeBlockImpl(VM& vm, const SourceCode& s
bool isInsideOrdinaryFunction = executable && executable->isInsideOrdinaryFunction();

std::unique_ptr<RootNode> rootNode = parse<RootNode>(
vm, source, Identifier(), ImplementationVisibility::Public, JSParserBuiltinMode::NotBuiltin, strictMode, scriptMode, CacheTypes<UnlinkedCodeBlockType>::parseMode, SuperBinding::NotNeeded, error, nullptr, ConstructorKind::None, derivedContextType, evalContextType, nullptr, privateNameEnvironment, nullptr, isInsideOrdinaryFunction);
vm, source, Identifier(), ImplementationVisibility::Public, JSParserBuiltinMode::NotBuiltin, strictMode, scriptMode, CacheTypes<UnlinkedCodeBlockType>::parseMode, FunctionMode::None, SuperBinding::NotNeeded, error, nullptr, ConstructorKind::None, derivedContextType, evalContextType, nullptr, privateNameEnvironment, nullptr, isInsideOrdinaryFunction);

if (!rootNode)
return nullptr;
Expand Down
4 changes: 2 additions & 2 deletions Source/JavaScriptCore/runtime/Completion.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ static inline bool checkSyntaxInternal(VM& vm, const SourceCode& source, ParserE
{
return !!parse<ProgramNode>(
vm, source, Identifier(), ImplementationVisibility::Public, JSParserBuiltinMode::NotBuiltin,
JSParserStrictMode::NotStrict, JSParserScriptMode::Classic, SourceParseMode::ProgramMode, SuperBinding::NotNeeded, error);
JSParserStrictMode::NotStrict, JSParserScriptMode::Classic, SourceParseMode::ProgramMode, FunctionMode::None, SuperBinding::NotNeeded, error);
}

bool checkSyntax(JSGlobalObject* globalObject, const SourceCode& source, JSValue* returnedException)
Expand Down Expand Up @@ -77,7 +77,7 @@ bool checkModuleSyntax(JSGlobalObject* globalObject, const SourceCode& source, P
RELEASE_ASSERT(vm.atomStringTable() == Thread::current().atomStringTable());
std::unique_ptr<ModuleProgramNode> moduleProgramNode = parse<ModuleProgramNode>(
vm, source, Identifier(), ImplementationVisibility::Public, JSParserBuiltinMode::NotBuiltin,
JSParserStrictMode::Strict, JSParserScriptMode::Module, SourceParseMode::ModuleAnalyzeMode, SuperBinding::NotNeeded, error);
JSParserStrictMode::Strict, JSParserScriptMode::Module, SourceParseMode::ModuleAnalyzeMode, FunctionMode::None, SuperBinding::NotNeeded, error);
if (!moduleProgramNode)
return false;

Expand Down
2 changes: 1 addition & 1 deletion Source/JavaScriptCore/runtime/JSModuleLoader.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -363,7 +363,7 @@ JSC_DEFINE_HOST_FUNCTION(moduleLoaderParseModule, (JSGlobalObject* globalObject,
ParserError error;
std::unique_ptr<ModuleProgramNode> moduleProgramNode = parse<ModuleProgramNode>(
vm, sourceCode, Identifier(), ImplementationVisibility::Public, JSParserBuiltinMode::NotBuiltin,
JSParserStrictMode::Strict, JSParserScriptMode::Module, SourceParseMode::ModuleAnalyzeMode, SuperBinding::NotNeeded, error);
JSParserStrictMode::Strict, JSParserScriptMode::Module, SourceParseMode::ModuleAnalyzeMode, FunctionMode::None, SuperBinding::NotNeeded, error);
if (error.isValid())
RELEASE_AND_RETURN(scope, JSValue::encode(rejectWithError(error.toErrorObject(globalObject, sourceCode))));
ASSERT(moduleProgramNode);
Expand Down

0 comments on commit 3e0dc21

Please sign in to comment.