From dc8817638ecc30b8a5ede041327f903671374aa5 Mon Sep 17 00:00:00 2001
From: Andreas Kling <kling@serenityos.org>
Date: Sun, 21 Mar 2021 17:14:20 +0100
Subject: [PATCH] LibJS: Only update anonymous function names when necessary

Previously we would generate function names for anonymous functions
on every AssignmentExpression, even if we weren't assigning a function.

We were also setting names of anonymous functions in arrays, which is
apparently a SpiderMonkey specific behavior not supported by V8, JSC
or required by ECMA262. This patch removes that behavior.

This is a huge performance improvement on the CanvasCycle demo! :^)
---
 Userland/Libraries/LibJS/AST.cpp              | 33 ++++++-------------
 .../LibJS/Tests/functions/function-name.js    | 13 ++------
 2 files changed, 13 insertions(+), 33 deletions(-)

diff --git a/Userland/Libraries/LibJS/AST.cpp b/Userland/Libraries/LibJS/AST.cpp
index 58d98faae4..838b93e86d 100644
--- a/Userland/Libraries/LibJS/AST.cpp
+++ b/Userland/Libraries/LibJS/AST.cpp
@@ -80,30 +80,13 @@ String ASTNode::class_name() const
     return demangle(typeid(*this).name()).substring(4);
 }
 
-static void update_function_name(Value value, const FlyString& name, HashTable<JS::Cell*>& visited)
-{
-    if (!value.is_object())
-        return;
-    if (visited.contains(value.as_cell()))
-        return;
-    visited.set(value.as_cell());
-    auto& object = value.as_object();
-    if (object.is_function()) {
-        auto& function = static_cast<Function&>(object);
-        if (is<ScriptFunction>(function) && function.name().is_empty())
-            static_cast<ScriptFunction&>(function).set_name(name);
-    } else if (object.is_array()) {
-        auto& array = static_cast<Array&>(object);
-        array.indexed_properties().for_each_value([&](auto& array_element_value) {
-            update_function_name(array_element_value, name, visited);
-        });
-    }
-}
-
 static void update_function_name(Value value, const FlyString& name)
 {
-    HashTable<JS::Cell*> visited;
-    update_function_name(value, name, visited);
+    if (!value.is_function())
+        return;
+    auto& function = value.as_function();
+    if (is<ScriptFunction>(function) && function.name().is_empty())
+        static_cast<ScriptFunction&>(function).set_name(name);
 }
 
 static String get_function_name(GlobalObject& global_object, Value value)
@@ -1443,7 +1426,11 @@ Value AssignmentExpression::execute(Interpreter& interpreter, GlobalObject& glob
         interpreter.vm().throw_exception<ReferenceError>(global_object, ErrorType::InvalidLeftHandAssignment);
         return {};
     }
-    update_function_name(rhs_result, get_function_name(global_object, reference.name().to_value(interpreter.vm())));
+
+    // FIXME: We should also check if the LHS is an identifier reference.
+    if (rhs_result.is_function())
+        update_function_name(rhs_result, get_function_name(global_object, reference.name().to_value(interpreter.vm())));
+
     reference.put(global_object, rhs_result);
 
     if (interpreter.exception())
diff --git a/Userland/Libraries/LibJS/Tests/functions/function-name.js b/Userland/Libraries/LibJS/Tests/functions/function-name.js
index 90520187d9..4402abff40 100644
--- a/Userland/Libraries/LibJS/Tests/functions/function-name.js
+++ b/Userland/Libraries/LibJS/Tests/functions/function-name.js
@@ -21,9 +21,9 @@ test("function assigned to variable", () => {
 
 test("functions in array assigned to variable", () => {
     const arr = [function () {}, function () {}, function () {}];
-    expect(arr[0].name).toBe("arr");
-    expect(arr[1].name).toBe("arr");
-    expect(arr[2].name).toBe("arr");
+    expect(arr[0].name).toBe("");
+    expect(arr[1].name).toBe("");
+    expect(arr[2].name).toBe("");
 });
 
 test("functions in objects", () => {
@@ -48,10 +48,3 @@ test("names of native functions", () => {
     expect((console.debug.name = "warn")).toBe("warn");
     expect(console.debug.name).toBe("debug");
 });
-
-test("cyclic members should not cause infinite recursion (#3471)", () => {
-    let a = [() => 4];
-    a[1] = a;
-    a = a;
-    expect(a[0].name).toBe("a");
-});