Bug 19718: Named anonymous functions are slow accessing global variables

<https://bugs.webkit.org/show_bug.cgi?id=19718>

Reviewed by Cameron Zwarich.

To fix this we switch over to an activation-like scope object for
on which we attach the function name property, and add logic to
prevent cross scope assignment to read only properties.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@35368 268f45cc-cd09-0410-ab3c-d52691b4dbfc

diff --git a/JavaScriptCore/ChangeLog b/JavaScriptCore/ChangeLog
index 67d501f..74e991b 100644 (file)
--- a/JavaScriptCore/ChangeLog
+++ b/JavaScriptCore/ChangeLog
@@ -1,3 +1,39 @@
+2008-07-25  Oliver Hunt  <oliver@apple.com>
+
+        Reviewed by Cameron Zwarich.
+
+        Bug 19718: Named anonymous functions are slow accessing global variables
+        <https://bugs.webkit.org/show_bug.cgi?id=19718>
+
+        To fix this we switch over to an activation-like scope object for
+        on which we attach the function name property, and add logic to 
+        prevent cross scope assignment to read only properties.
+
+        * GNUmakefile.am:
+        * JavaScriptCore.pri:
+        * JavaScriptCore.vcproj/JavaScriptCore/JavaScriptCore.vcproj:
+        * JavaScriptCore.xcodeproj/project.pbxproj:
+        * VM/CodeGenerator.cpp:
+        (KJS::CodeGenerator::findScopedProperty):
+        (KJS::CodeGenerator::emitResolve):
+        * VM/CodeGenerator.h:
+        * kjs/AllInOneFile.cpp:
+        * kjs/JSStaticScopeObject.cpp: Added.
+        (KJS::JSStaticScopeObject::putWithAttributes):
+        (KJS::JSStaticScopeObject::isDynamicScope):
+        (KJS::JSStaticScopeObject::~JSStaticScopeObject):
+        (KJS::JSStaticScopeObject::getOwnPropertySlot):
+        * kjs/JSStaticScopeObject.h: Added.
+        (KJS::JSStaticScopeObject::JSStaticScopeObjectData::JSStaticScopeObjectData):
+        (KJS::JSStaticScopeObject::JSStaticScopeObject):
+        * kjs/nodes.cpp:
+        (KJS::FunctionCallResolveNode::emitCode):
+        (KJS::PostfixResolveNode::emitCode):
+        (KJS::PrefixResolveNode::emitCode):
+        (KJS::ReadModifyResolveNode::emitCode):
+        (KJS::AssignResolveNode::emitCode):
+        (KJS::FuncExprNode::makeFunction):
+
 2008-07-25  kevino  <kevino@theolliviers.com>
 
         wx build fix for Win.

diff --git a/JavaScriptCore/GNUmakefile.am b/JavaScriptCore/GNUmakefile.am
index c690fcc..7d5c550 100644 (file)
--- a/JavaScriptCore/GNUmakefile.am
+++ b/JavaScriptCore/GNUmakefile.am
@@ -116,6 +116,7 @@ javascriptcore_sources += \
        JavaScriptCore/kjs/JSLock.cpp \
        JavaScriptCore/kjs/JSNumberCell.cpp \
        JavaScriptCore/kjs/JSObject.cpp \
+       JavaScriptCore/kjs/JSStaticScopeObject.cpp \
        JavaScriptCore/kjs/JSString.cpp \
        JavaScriptCore/kjs/JSValue.cpp \
        JavaScriptCore/kjs/JSVariableObject.cpp \

diff --git a/JavaScriptCore/JavaScriptCore.pri b/JavaScriptCore/JavaScriptCore.pri
index 726569c..fbe8e44 100644 (file)
--- a/JavaScriptCore/JavaScriptCore.pri
+++ b/JavaScriptCore/JavaScriptCore.pri
@@ -48,6 +48,7 @@ SOURCES += \
     kjs/InitializeThreading.cpp \
     kjs/JSGlobalData.cpp \
     kjs/JSGlobalObject.cpp \
+    kjs/JSStaticScopeObject.cpp \
     kjs/JSVariableObject.cpp \
     kjs/JSActivation.cpp \
     kjs/JSNotAnObject.cpp \

diff --git a/JavaScriptCore/JavaScriptCore.vcproj/JavaScriptCore/JavaScriptCore.vcproj b/JavaScriptCore/JavaScriptCore.vcproj/JavaScriptCore/JavaScriptCore.vcproj
index 857eba1..b66811a 100644 (file)
--- a/JavaScriptCore/JavaScriptCore.vcproj/JavaScriptCore/JavaScriptCore.vcproj
+++ b/JavaScriptCore/JavaScriptCore.vcproj/JavaScriptCore/JavaScriptCore.vcproj
@@ -573,6 +573,14 @@
                                >\r
                        </File>\r
                        <File\r
+                               RelativePath="..\..\kjs\JSStaticScopeObject.cpp"\r
+                               >\r
+                       </File>\r
+                       <File\r
+                               RelativePath="..\..\kjs\JSStaticScopeObject.h"\r
+                               >\r
+                       </File>\r
+                       <File\r
                                RelativePath="..\..\kjs\JSGlobalObjectFunctions.cpp"\r
                                >\r
                        </File>\r

diff --git a/JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj b/JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj
index b19dd3b..c112857 100644 (file)
--- a/JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj
+++ b/JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj
@@ -609,6 +609,8 @@
                A727FF650DA3053B00E548D7 /* JSPropertyNameIterator.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; name = JSPropertyNameIterator.h; path = VM/JSPropertyNameIterator.h; sourceTree = "<group>"; };
                A727FF660DA3053B00E548D7 /* JSPropertyNameIterator.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; name = JSPropertyNameIterator.cpp; path = VM/JSPropertyNameIterator.cpp; sourceTree = "<group>"; };
                A7C31DA80DBEBA4300FDF8EB /* SegmentedVector.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; name = SegmentedVector.h; path = VM/SegmentedVector.h; sourceTree = "<group>"; };
+               A7E42C180E3938830065A544 /* JSStaticScopeObject.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = JSStaticScopeObject.h; sourceTree = "<group>"; };
+               A7E42C190E3938830065A544 /* JSStaticScopeObject.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; path = JSStaticScopeObject.cpp; sourceTree = "<group>"; };
                A8E894310CD0602400367179 /* JSCallbackObjectFunctions.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = JSCallbackObjectFunctions.h; sourceTree = "<group>"; };
                A8E894330CD0603F00367179 /* JSGlobalObject.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = JSGlobalObject.h; sourceTree = "<group>"; };
                BC02E9040E1839DB000F9297 /* ErrorConstructor.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; path = ErrorConstructor.cpp; sourceTree = "<group>"; };
@@ -1166,6 +1168,8 @@
                                BC7F8FB80E19D1C3008632C0 /* JSNumberCell.h */,
                                BC22A3980E16E14800AF21C8 /* JSObject.cpp */,
                                BC22A3990E16E14800AF21C8 /* JSObject.h */,
+                               A7E42C180E3938830065A544 /* JSStaticScopeObject.h */,
+                               A7E42C190E3938830065A544 /* JSStaticScopeObject.cpp */,
                                BC02E9B60E1842FA000F9297 /* JSString.cpp */,
                                F692A8620255597D01FF60F7 /* JSString.h */,
                                14ABB454099C2A0F00E2A24F /* JSType.h */,

diff --git a/JavaScriptCore/VM/CodeGenerator.cpp b/JavaScriptCore/VM/CodeGenerator.cpp
index c53d61d..abb5221 100644 (file)
--- a/JavaScriptCore/VM/CodeGenerator.cpp
+++ b/JavaScriptCore/VM/CodeGenerator.cpp
@@ -622,7 +622,7 @@ RegisterID* CodeGenerator::emitNullaryOp(OpcodeID opcode, RegisterID* dst)
     return dst;
 }
 
-bool CodeGenerator::findScopedProperty(const Identifier& property, int& index, size_t& stackDepth)
+bool CodeGenerator::findScopedProperty(const Identifier& property, int& index, size_t& stackDepth, bool forWriting)
 {
     // Cases where we cannot optimise the lookup
     if (property == propertyNames().arguments || !canOptimizeNonLocals()) {
@@ -644,6 +644,11 @@ bool CodeGenerator::findScopedProperty(const Identifier& property, int& index, s
 
         // Found the property
         if (!entry.isNull()) {
+            if (entry.isReadOnly() && forWriting) {
+                stackDepth = 0;
+                index = missingSymbolMarker();
+                return false;
+            }
             stackDepth = depth;
             index = entry.getIndex();
             return true;
@@ -662,7 +667,7 @@ RegisterID* CodeGenerator::emitResolve(RegisterID* dst, const Identifier& proper
 {
     size_t depth = 0;
     int index = 0;
-    if (!findScopedProperty(property, index, depth)) {
+    if (!findScopedProperty(property, index, depth, false)) {
         // We can't optimise at all :-(
         emitOpcode(op_resolve);
         instructions().append(dst->index());

diff --git a/JavaScriptCore/VM/CodeGenerator.h b/JavaScriptCore/VM/CodeGenerator.h
index 5d18860..af1e44e 100644 (file)
--- a/JavaScriptCore/VM/CodeGenerator.h
+++ b/JavaScriptCore/VM/CodeGenerator.h
@@ -104,7 +104,7 @@ namespace KJS {
         //
         // NB: depth does _not_ include the local scope.  eg. a depth of 0 refers
         // to the scope containing this codeblock.
-        bool findScopedProperty(const Identifier&, int& index, size_t& depth);
+        bool findScopedProperty(const Identifier&, int& index, size_t& depth, bool forWriting);
 
         // Returns the register storing "this"
         RegisterID* thisRegister() { return &m_thisRegister; }

diff --git a/JavaScriptCore/kjs/AllInOneFile.cpp b/JavaScriptCore/kjs/AllInOneFile.cpp
index f949aed..cba4e79 100644 (file)
--- a/JavaScriptCore/kjs/AllInOneFile.cpp
+++ b/JavaScriptCore/kjs/AllInOneFile.cpp
@@ -26,6 +26,7 @@
 #define JAVASCRIPTCORE_BUILDING_ALL_IN_ONE_FILE 1
 #include "config.h"
 
+#include "JSStaticScopeObject.cpp"
 #include "JSFunction.cpp"
 #include "IndexToNameMap.cpp"
 #include "Arguments.cpp"

diff --git a/JavaScriptCore/kjs/JSStaticScopeObject.cpp b/JavaScriptCore/kjs/JSStaticScopeObject.cpp
new file mode 100644 (file)
index 0000000..1cdc1ce
--- /dev/null
+++ b/JavaScriptCore/kjs/JSStaticScopeObject.cpp
@@ -0,0 +1,61 @@
+/*
+ * Copyright (C) 2008 Apple Inc. All Rights Reserved.
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions
+ * are met:
+ * 1. Redistributions of source code must retain the above copyright
+ *    notice, this list of conditions and the following disclaimer.
+ * 2. Redistributions in binary form must reproduce the above copyright
+ *    notice, this list of conditions and the following disclaimer in the
+ *    documentation and/or other materials provided with the distribution.
+ *
+ * THIS SOFTWARE IS PROVIDED BY APPLE INC. ``AS IS'' AND ANY
+ * EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
+ * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR
+ * PURPOSE ARE DISCLAIMED.  IN NO EVENT SHALL APPLE INC. OR
+ * CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL,
+ * EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO,
+ * PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR
+ * PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY
+ * OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+ * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
+ * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. 
+ */
+
+#include "config.h"
+
+#include "JSStaticScopeObject.h"
+
+namespace KJS {
+
+void JSStaticScopeObject::putWithAttributes(ExecState*, const Identifier& propertyName, JSValue* value, unsigned attributes)
+{
+    if (symbolTablePutWithAttributes(propertyName, value, attributes))
+        return;
+    
+    ASSERT_NOT_REACHED();
+}
+
+bool JSStaticScopeObject::isDynamicScope() const
+{
+    return false;
+}
+
+JSStaticScopeObject::~JSStaticScopeObject()
+{
+    ASSERT(d);
+    delete static_cast<JSStaticScopeObjectData*>(d);
+}
+
+inline bool JSStaticScopeObject::getOwnPropertySlot(ExecState*, const Identifier& propertyName, PropertySlot& slot)
+{
+    return symbolTableGet(propertyName, slot);
+}
+
+inline bool JSStaticScopeObject::getOwnPropertySlot(ExecState*, const Identifier& propertyName, PropertySlot& slot, bool& slotIsWriteable)
+{
+    return symbolTableGet(propertyName, slot, slotIsWriteable);
+}
+
+}

diff --git a/JavaScriptCore/kjs/JSStaticScopeObject.h b/JavaScriptCore/kjs/JSStaticScopeObject.h
new file mode 100644 (file)
index 0000000..2cabcea
--- /dev/null
+++ b/JavaScriptCore/kjs/JSStaticScopeObject.h
@@ -0,0 +1,63 @@
+/*
+ * Copyright (C) 2008 Apple Inc. All Rights Reserved.
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions
+ * are met:
+ * 1. Redistributions of source code must retain the above copyright
+ *    notice, this list of conditions and the following disclaimer.
+ * 2. Redistributions in binary form must reproduce the above copyright
+ *    notice, this list of conditions and the following disclaimer in the
+ *    documentation and/or other materials provided with the distribution.
+ *
+ * THIS SOFTWARE IS PROVIDED BY APPLE INC. ``AS IS'' AND ANY
+ * EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
+ * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR
+ * PURPOSE ARE DISCLAIMED.  IN NO EVENT SHALL APPLE INC. OR
+ * CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL,
+ * EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO,
+ * PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR
+ * PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY
+ * OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+ * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
+ * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. 
+ */
+
+#ifndef JSStaticScopeObject_h
+#define JSStaticScopeObject_h
+
+#include "JSVariableObject.h"
+
+namespace KJS{
+    
+    class JSStaticScopeObject : public JSVariableObject {
+    protected:
+        using JSVariableObject::JSVariableObjectData;
+        struct JSStaticScopeObjectData : public JSVariableObjectData {
+            JSStaticScopeObjectData()
+                : JSVariableObjectData(&symbolTable, &registerStore + 1)
+            {
+                registerArraySize = 1;
+            }
+            SymbolTable symbolTable;
+            Register registerStore;
+        };
+        
+    public:
+        JSStaticScopeObject(const Identifier& ident, JSValue* value, unsigned attributes)
+            : JSVariableObject(new JSStaticScopeObjectData())
+        {
+            JSStaticScopeObjectData* data = static_cast<JSStaticScopeObjectData*>(d);
+            data->registerStore = value;
+            symbolTable().add(ident.ustring().rep(), SymbolTableEntry(-1, attributes));
+        }
+        virtual ~JSStaticScopeObject();
+        bool isDynamicScope() const;
+        virtual bool getOwnPropertySlot(ExecState*, const Identifier&, PropertySlot&);
+        virtual bool getOwnPropertySlot(ExecState*, const Identifier&, PropertySlot&, bool& slotIsWriteable);
+        void putWithAttributes(ExecState*, const Identifier&, JSValue*, unsigned attributes);
+    };
+
+}
+
+#endif // !JSStaticScopeObject_h

diff --git a/JavaScriptCore/kjs/nodes.cpp b/JavaScriptCore/kjs/nodes.cpp
index 406777f..c01b7fb 100644 (file)
--- a/JavaScriptCore/kjs/nodes.cpp
+++ b/JavaScriptCore/kjs/nodes.cpp
@@ -29,6 +29,7 @@
 #include "CodeGenerator.h"
 #include "ExecState.h"
 #include "JSGlobalObject.h"
+#include "JSStaticScopeObject.h"
 #include "Parser.h"
 #include "PropertyNameArray.h"
 #include "RegExpObject.h"
@@ -428,7 +429,7 @@ RegisterID* FunctionCallResolveNode::emitCode(CodeGenerator& generator, Register
 
     int index = 0;
     size_t depth = 0;
-    if (generator.findScopedProperty(m_ident, index, depth) && index != missingSymbolMarker()) {
+    if (generator.findScopedProperty(m_ident, index, depth, false) && index != missingSymbolMarker()) {
         RegisterID* func = generator.emitGetScopedVar(generator.newTemporary(), depth, index);
         return generator.emitCall(generator.finalDestination(dst), func, 0, m_args.get(), m_divot, m_startOffset, m_endOffset);
     }
@@ -486,7 +487,7 @@ RegisterID* PostfixResolveNode::emitCode(CodeGenerator& generator, RegisterID* d
 
     int index = 0;
     size_t depth = 0;
-    if (generator.findScopedProperty(m_ident, index, depth) && index != missingSymbolMarker()) {
+    if (generator.findScopedProperty(m_ident, index, depth, true) && index != missingSymbolMarker()) {
         RefPtr<RegisterID> value = generator.emitGetScopedVar(generator.newTemporary(), depth, index);
         RegisterID* oldValue;
         if (dst == ignoredResult()) {
@@ -669,7 +670,7 @@ RegisterID* PrefixResolveNode::emitCode(CodeGenerator& generator, RegisterID* ds
 
     int index = 0;
     size_t depth = 0;
-    if (generator.findScopedProperty(m_ident, index, depth) && index != missingSymbolMarker()) {
+    if (generator.findScopedProperty(m_ident, index, depth, false) && index != missingSymbolMarker()) {
         RefPtr<RegisterID> propDst = generator.emitGetScopedVar(generator.tempDestination(dst), depth, index);
         emitPreIncOrDec(generator, propDst.get(), m_operator);
         generator.emitPutScopedVar(depth, index, propDst.get());
@@ -872,7 +873,7 @@ RegisterID* ReadModifyResolveNode::emitCode(CodeGenerator& generator, RegisterID
 
     int index = 0;
     size_t depth = 0;
-    if (generator.findScopedProperty(m_ident, index, depth) && index != missingSymbolMarker()) {
+    if (generator.findScopedProperty(m_ident, index, depth, true) && index != missingSymbolMarker()) {
         RefPtr<RegisterID> src1 = generator.emitGetScopedVar(generator.tempDestination(dst), depth, index);
         RegisterID* src2 = generator.emitNode(m_right.get());
         RegisterID* result = emitReadModifyAssignment(generator, generator.finalDestination(dst, src1.get()), src1.get(), src2, m_operator);
@@ -903,7 +904,7 @@ RegisterID* AssignResolveNode::emitCode(CodeGenerator& generator, RegisterID* ds
 
     int index = 0;
     size_t depth = 0;
-    if (generator.findScopedProperty(m_ident, index, depth) && index != missingSymbolMarker()) {
+    if (generator.findScopedProperty(m_ident, index, depth, true) && index != missingSymbolMarker()) {
         if (dst == ignoredResult())
             dst = 0;
         RegisterID* value = generator.emitNode(dst, m_right.get());
@@ -1839,8 +1840,7 @@ JSFunction* FuncExprNode::makeFunction(ExecState* exec, ScopeChainNode* scopeCha
      */
 
     if (!m_ident.isNull()) {
-        JSObject* functionScopeObject = new (exec) JSObject;
-        functionScopeObject->putDirect(m_ident, func, ReadOnly | DontDelete);
+        JSStaticScopeObject* functionScopeObject = new (exec) JSStaticScopeObject(m_ident, func, ReadOnly | DontDelete);
         func->scope().push(functionScopeObject);
     }
 

diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
index d7354e9..6bf1bda 100644 (file)
--- a/LayoutTests/ChangeLog
+++ b/LayoutTests/ChangeLog
@@ -1,3 +1,13 @@
+2008-07-25  Oliver Hunt  <oliver@apple.com>
+
+        Reviewed by Cameron Zwarich.
+
+        Layout test covering assignment to read only properties
+        across scope boundaries.
+
+        * fast/js/const-expected.txt:
+        * fast/js/resources/const.js:
+
 2008-07-25  Dan Bernstein  <mitz@apple.com>
 
         Rubber-stamped by Sam Weinig.

diff --git a/LayoutTests/fast/js/const-expected.txt b/LayoutTests/fast/js/const-expected.txt
index 665cd30..59fd8a5 100644 (file)
--- a/LayoutTests/fast/js/const-expected.txt
+++ b/LayoutTests/fast/js/const-expected.txt
@@ -45,6 +45,8 @@ PASS one = 2 is 2
 PASS one is 1
 PASS object.inWith1 is 'RIGHT'
 PASS inWith2 is 'RIGHT'
+PASS (function(){ one = 2; return one; })() is 1
+PASS f() is f
 PASS successfullyParsed is true
 
 TEST COMPLETE

diff --git a/LayoutTests/fast/js/resources/const.js b/LayoutTests/fast/js/resources/const.js
index 623ba2c..4bade68 100644 (file)
--- a/LayoutTests/fast/js/resources/const.js
+++ b/LayoutTests/fast/js/resources/const.js
@@ -103,4 +103,8 @@ with (object) {
 shouldBe("object.inWith1", "'RIGHT'");
 shouldBe("inWith2", "'RIGHT'");
 
+shouldBe("(function(){ one = 2; return one; })()", "1")
+var f = function g() { g="FAIL"; return g; };
+shouldBe("f()", "f");
+
 var successfullyParsed = true;