Don't waste memory for error.stack
authorfpizlo@apple.com <fpizlo@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sat, 10 Feb 2018 23:49:54 +0000 (23:49 +0000)
committerfpizlo@apple.com <fpizlo@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sat, 10 Feb 2018 23:49:54 +0000 (23:49 +0000)
https://bugs.webkit.org/show_bug.cgi?id=182656

Reviewed by Saam Barati.

JSTests:

Tests the policy.

* stress/gc-error-stack.js: Added. Shows that the GC forgets frames now.
* stress/no-gc-error-stack.js: Added. Shows that the GC won't forget things if you ask for the stack.

Source/JavaScriptCore:

This makes the StackFrames in ErrorInstance and Exception weak. We simply forget their
contents if we GC.

This isn't going to happen under normal operation since your callees and code blocks will
still be alive when you ask for .stack.

Bug 182650 tracks improving this so that it's not lossy. For now, I think it's worth it,
since it is likely to recover 3-5 MB on membuster.

* heap/Heap.cpp:
(JSC::Heap::finalizeUnconditionalFinalizers):
* runtime/ErrorInstance.cpp:
(JSC::ErrorInstance::visitChildren):
(JSC::ErrorInstance::finalizeUnconditionally):
* runtime/ErrorInstance.h:
(JSC::ErrorInstance::subspaceFor):
* runtime/Exception.cpp:
(JSC::Exception::visitChildren):
(JSC::Exception::finalizeUnconditionally):
* runtime/Exception.h:
(JSC::Exception::valueOffset): Deleted.
(JSC::Exception::value const): Deleted.
(JSC::Exception::stack const): Deleted.
(JSC::Exception::didNotifyInspectorOfThrow const): Deleted.
(JSC::Exception::setDidNotifyInspectorOfThrow): Deleted.
* runtime/StackFrame.cpp:
(JSC::StackFrame::isFinalizationCandidate):
(JSC::StackFrame::finalizeUnconditionally):
(JSC::StackFrame::visitChildren): Deleted.
* runtime/StackFrame.h:
* runtime/VM.cpp:
(JSC::VM::VM):
* runtime/VM.h:

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

13 files changed:
JSTests/ChangeLog
JSTests/stress/gc-error-stack.js [new file with mode: 0644]
JSTests/stress/no-gc-error-stack.js [new file with mode: 0644]
Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/heap/Heap.cpp
Source/JavaScriptCore/runtime/ErrorInstance.cpp
Source/JavaScriptCore/runtime/ErrorInstance.h
Source/JavaScriptCore/runtime/Exception.cpp
Source/JavaScriptCore/runtime/Exception.h
Source/JavaScriptCore/runtime/StackFrame.cpp
Source/JavaScriptCore/runtime/StackFrame.h
Source/JavaScriptCore/runtime/VM.cpp
Source/JavaScriptCore/runtime/VM.h

index 40030b6..cc2570b 100644 (file)
@@ -1,3 +1,15 @@
+2018-02-09  Filip Pizlo  <fpizlo@apple.com>
+
+        Don't waste memory for error.stack
+        https://bugs.webkit.org/show_bug.cgi?id=182656
+
+        Reviewed by Saam Barati.
+        
+        Tests the policy.
+
+        * stress/gc-error-stack.js: Added. Shows that the GC forgets frames now.
+        * stress/no-gc-error-stack.js: Added. Shows that the GC won't forget things if you ask for the stack.
+
 2018-02-08  Yusuke Suzuki  <utatane.tea@gmail.com>
 
         [JSC] Update Test262 to Feb 9 version
diff --git a/JSTests/stress/gc-error-stack.js b/JSTests/stress/gc-error-stack.js
new file mode 100644 (file)
index 0000000..6fef59e
--- /dev/null
@@ -0,0 +1,18 @@
+"use strict";
+
+var errors = [];
+
+for (let i = 0; i < 1000; ++i)
+    eval("(function foo" + i + "() { errors.push(new Error()); })();");
+
+gc();
+
+let didGCAtLeastOne = false;
+for (let error of errors) {
+    if (!error.stack.startsWith("foo"))
+        didGCAtLeastOne = true;
+}
+
+if (!didGCAtLeastOne)
+    throw new Error("Bad result: didGCAtLeastOne = " + didGCAtLeastOne);
+
diff --git a/JSTests/stress/no-gc-error-stack.js b/JSTests/stress/no-gc-error-stack.js
new file mode 100644 (file)
index 0000000..31eb803
--- /dev/null
@@ -0,0 +1,22 @@
+//@ runDefault
+
+"use strict";
+
+var errors = [];
+
+for (let i = 0; i < 1000; ++i)
+    eval("(function foo" + i + "() { errors.push(new Error()); })();");
+
+for (let error of errors)
+    error.stack;
+
+gc();
+
+for (let error of errors) {
+    if (!error.stack.startsWith("foo")) {
+        print("ERROR: Stack does not begin with foo: " + error.stack);
+        throw new Error("fail");
+    }
+}
+
+
index 92549db..183a044 100644 (file)
@@ -1,3 +1,44 @@
+2018-02-09  Filip Pizlo  <fpizlo@apple.com>
+
+        Don't waste memory for error.stack
+        https://bugs.webkit.org/show_bug.cgi?id=182656
+
+        Reviewed by Saam Barati.
+        
+        This makes the StackFrames in ErrorInstance and Exception weak. We simply forget their
+        contents if we GC.
+        
+        This isn't going to happen under normal operation since your callees and code blocks will
+        still be alive when you ask for .stack.
+        
+        Bug 182650 tracks improving this so that it's not lossy. For now, I think it's worth it,
+        since it is likely to recover 3-5 MB on membuster.
+
+        * heap/Heap.cpp:
+        (JSC::Heap::finalizeUnconditionalFinalizers):
+        * runtime/ErrorInstance.cpp:
+        (JSC::ErrorInstance::visitChildren):
+        (JSC::ErrorInstance::finalizeUnconditionally):
+        * runtime/ErrorInstance.h:
+        (JSC::ErrorInstance::subspaceFor):
+        * runtime/Exception.cpp:
+        (JSC::Exception::visitChildren):
+        (JSC::Exception::finalizeUnconditionally):
+        * runtime/Exception.h:
+        (JSC::Exception::valueOffset): Deleted.
+        (JSC::Exception::value const): Deleted.
+        (JSC::Exception::stack const): Deleted.
+        (JSC::Exception::didNotifyInspectorOfThrow const): Deleted.
+        (JSC::Exception::setDidNotifyInspectorOfThrow): Deleted.
+        * runtime/StackFrame.cpp:
+        (JSC::StackFrame::isFinalizationCandidate):
+        (JSC::StackFrame::finalizeUnconditionally):
+        (JSC::StackFrame::visitChildren): Deleted.
+        * runtime/StackFrame.h:
+        * runtime/VM.cpp:
+        (JSC::VM::VM):
+        * runtime/VM.h:
+
 2018-02-09  Carlos Alberto Lopez Perez  <clopez@igalia.com>
 
         Fix build on ARMv7 traditional JSCOnly bot after r228306
index 988be1d..1cd041e 100644 (file)
@@ -579,6 +579,8 @@ void Heap::finalizeMarkedUnconditionalFinalizers(CellSet& cellSet)
 
 void Heap::finalizeUnconditionalFinalizers()
 {
+    finalizeMarkedUnconditionalFinalizers<ErrorInstance>(vm()->errorInstancesWithFinalizers);
+    finalizeMarkedUnconditionalFinalizers<Exception>(vm()->exceptionsWithFinalizers);
     finalizeMarkedUnconditionalFinalizers<InferredType>(vm()->inferredTypesWithFinalizers);
     finalizeMarkedUnconditionalFinalizers<InferredValue>(vm()->inferredValuesWithFinalizers);
     vm()->forEachCodeBlockSpace(
index c172428..8f418ae 100644 (file)
@@ -1,6 +1,6 @@
 /*
  *  Copyright (C) 1999-2000 Harri Porten (porten@kde.org)
- *  Copyright (C) 2003-2017 Apple Inc. All rights reserved.
+ *  Copyright (C) 2003-2018 Apple Inc. All rights reserved.
  *
  *  This library is free software; you can redistribute it and/or
  *  modify it under the terms of the GNU Lesser General Public
@@ -23,6 +23,7 @@
 
 #include "CodeBlock.h"
 #include "InlineCallFrame.h"
+#include "IsoCellSetInlines.h"
 #include "JSScope.h"
 #include "JSCInlines.h"
 #include "ParseInt.h"
@@ -233,13 +234,33 @@ void ErrorInstance::visitChildren(JSCell* cell, SlotVisitor& visitor)
     ASSERT_GC_OBJECT_INHERITS(thisObject, info());
     Base::visitChildren(thisObject, visitor);
 
+    bool isFinalizationCandidate = false;
     {
         auto locker = holdLock(thisObject->cellLock());
         if (thisObject->m_stackTrace) {
-            for (StackFrame& frame : *thisObject->m_stackTrace)
-                frame.visitChildren(visitor);
+            for (StackFrame& frame : *thisObject->m_stackTrace) {
+                if (frame.isFinalizationCandidate()) {
+                    isFinalizationCandidate = true;
+                    break;
+                }
+            }
         }
     }
+    if (isFinalizationCandidate)
+        visitor.vm().errorInstancesWithFinalizers.add(thisObject);
+}
+
+void ErrorInstance::finalizeUnconditionally(VM& vm)
+{
+    {
+        auto locker = holdLock(cellLock());
+        if (m_stackTrace) {
+            for (StackFrame& frame : *m_stackTrace)
+                frame.finalizeUnconditionally(vm);
+        }
+    }
+    
+    vm.errorInstancesWithFinalizers.remove(this);
 }
 
 bool ErrorInstance::getOwnPropertySlot(JSObject* object, ExecState* exec, PropertyName propertyName, PropertySlot& slot)
index 56b3ae2..b70af4f 100644 (file)
@@ -1,6 +1,6 @@
 /*
  *  Copyright (C) 1999-2000 Harri Porten (porten@kde.org)
- *  Copyright (C) 2008-2017 Apple Inc. All rights reserved.
+ *  Copyright (C) 2008-2018 Apple Inc. All rights reserved.
  *
  *  This library is free software; you can redistribute it and/or
  *  modify it under the terms of the GNU Lesser General Public
 
 namespace JSC {
 
+// FIXME: This should be final, but isn't because of bizarre (and mostly wrong) things done in
+// WebAssembly.
+// https://bugs.webkit.org/show_bug.cgi?id=182649
 class ErrorInstance : public JSDestructibleObject {
 public:
+    template<typename CellType>
+    static IsoSubspace* subspaceFor(VM& vm)
+    {
+        return &vm.errorInstanceSpace;
+    }
+
     typedef JSDestructibleObject Base;
     const static unsigned StructureFlags = Base::StructureFlags | OverridesGetOwnPropertySlot | OverridesGetPropertyNames;
 
@@ -68,6 +77,8 @@ public:
     JS_EXPORT_PRIVATE String sanitizedToString(ExecState*);
     
     Vector<StackFrame>* stackTrace() { return m_stackTrace.get(); }
+    
+    void finalizeUnconditionally(VM&);
 
     bool materializeErrorInfoIfNeeded(VM&);
     bool materializeErrorInfoIfNeeded(VM&, PropertyName);
index bc989ef..0526750 100644 (file)
@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2015-2017 Apple Inc. All rights reserved.
+ * Copyright (C) 2015-2018 Apple Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
@@ -27,6 +27,7 @@
 #include "Exception.h"
 
 #include "Interpreter.h"
+#include "IsoCellSetInlines.h"
 #include "JSCInlines.h"
 
 namespace JSC {
@@ -57,9 +58,24 @@ void Exception::visitChildren(JSCell* cell, SlotVisitor& visitor)
     ASSERT_GC_OBJECT_INHERITS(thisObject, info());
     Base::visitChildren(thisObject, visitor);
 
+    bool isFinalizationCandidate = false;
     visitor.append(thisObject->m_value);
-    for (StackFrame& frame : thisObject->m_stack)
-        frame.visitChildren(visitor);
+    for (StackFrame& frame : thisObject->m_stack) {
+        if (frame.isFinalizationCandidate()) {
+            isFinalizationCandidate = true;
+            break;
+        }
+    }
+    if (isFinalizationCandidate)
+        visitor.vm().exceptionsWithFinalizers.add(thisObject);
+}
+
+void Exception::finalizeUnconditionally(VM& vm)
+{
+    for (StackFrame& frame : m_stack)
+        frame.finalizeUnconditionally(vm);
+    
+    vm.exceptionsWithFinalizers.remove(this);
 }
 
 Exception::Exception(VM& vm)
index 64d0a0a..cf5342b 100644 (file)
@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2015-2017 Apple Inc. All rights reserved.
+ * Copyright (C) 2015-2018 Apple Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
 
 namespace JSC {
     
-class Exception : public JSDestructibleObject {
+class Exception final : public JSDestructibleObject {
 public:
+    template<typename CellType>
+    static IsoSubspace* subspaceFor(VM& vm)
+    {
+        return &vm.exceptionSpace;
+    }
+
     typedef JSDestructibleObject Base;
     static const unsigned StructureFlags = StructureIsImmortal | Base::StructureFlags;
 
@@ -60,6 +66,8 @@ public:
     void setDidNotifyInspectorOfThrow() { m_didNotifyInspectorOfThrow = true; }
 
     ~Exception();
+    
+    void finalizeUnconditionally(VM&);
 
 private:
     Exception(VM&);
index 9f2d60d..15aefca 100644 (file)
@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2016-2017 Apple Inc. All rights reserved.
+ * Copyright (C) 2016-2018 Apple Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
@@ -142,12 +142,27 @@ String StackFrame::toString(VM& vm) const
     return traceBuild.toString().impl();
 }
 
-void StackFrame::visitChildren(SlotVisitor& visitor)
+bool StackFrame::isFinalizationCandidate()
 {
-    if (m_callee)
-        visitor.append(m_callee);
-    if (m_codeBlock)
-        visitor.append(m_codeBlock);
+    if (m_callee && !Heap::isMarked(m_callee.get()))
+        return true;
+    if (m_codeBlock && !Heap::isMarked(m_codeBlock.get()))
+        return true;
+    return false;
+}
+
+void StackFrame::finalizeUnconditionally(VM&)
+{
+    // FIXME: We should do something smarter. For example, if this happens, we could stringify the
+    // whole stack trace. The main shortcoming is that that requires doing operations that are not
+    // currently legal during finalization. We could make this work by giving JSC a proper "second
+    // chance" finalizer infrastructure. Or maybe there's an even easier way.
+    // https://bugs.webkit.org/show_bug.cgi?id=182650
+    
+    if (m_callee && !Heap::isMarked(m_callee.get()))
+        m_callee.clear();
+    if (m_codeBlock && !Heap::isMarked(m_codeBlock.get()))
+        m_codeBlock.clear();
 }
 
 } // namespace JSC
index 2877723..038290f 100644 (file)
@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2016-2017 Apple Inc. All rights reserved.
+ * Copyright (C) 2016-2018 Apple Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
@@ -56,7 +56,8 @@ public:
         return m_bytecodeOffset;
     }
     
-    void visitChildren(SlotVisitor&);
+    bool isFinalizationCandidate();
+    void finalizeUnconditionally(VM&);
 
 private:
     WriteBarrier<JSCell> m_callee { };
index 2a78c4a..2bf5b6f 100644 (file)
@@ -251,6 +251,8 @@ VM::VM(VMType vmType, HeapType heapType)
     , webAssemblyCodeBlockSpace("JSWebAssemblyCodeBlockSpace", heap, webAssemblyCodeBlockHeapCellType.get(), fastMallocAllocator.get())
 #endif
     , directEvalExecutableSpace ISO_SUBSPACE_INIT(heap, destructibleCellHeapCellType.get(), DirectEvalExecutable)
+    , errorInstanceSpace ISO_SUBSPACE_INIT(heap, destructibleObjectHeapCellType.get(), ErrorInstance)
+    , exceptionSpace ISO_SUBSPACE_INIT(heap, destructibleObjectHeapCellType.get(), Exception)
     , executableToCodeBlockEdgeSpace ISO_SUBSPACE_INIT(heap, cellHeapCellType.get(), ExecutableToCodeBlockEdge)
     , functionExecutableSpace ISO_SUBSPACE_INIT(heap, destructibleCellHeapCellType.get(), FunctionExecutable)
     , indirectEvalExecutableSpace ISO_SUBSPACE_INIT(heap, destructibleCellHeapCellType.get(), IndirectEvalExecutable)
@@ -264,6 +266,8 @@ VM::VM(VMType vmType, HeapType heapType)
     , structureSpace ISO_SUBSPACE_INIT(heap, destructibleCellHeapCellType.get(), Structure)
     , weakSetSpace ISO_SUBSPACE_INIT(heap, destructibleObjectHeapCellType.get(), JSWeakSet)
     , weakMapSpace ISO_SUBSPACE_INIT(heap, destructibleObjectHeapCellType.get(), JSWeakMap)
+    , errorInstancesWithFinalizers(errorInstanceSpace)
+    , exceptionsWithFinalizers(exceptionSpace)
     , executableToCodeBlockEdgesWithConstraints(executableToCodeBlockEdgeSpace)
     , executableToCodeBlockEdgesWithFinalizers(executableToCodeBlockEdgeSpace)
     , inferredTypesWithFinalizers(inferredTypeSpace)
index 50220d8..0a3251a 100644 (file)
@@ -339,6 +339,8 @@ public:
 #endif
     
     IsoSubspace directEvalExecutableSpace;
+    IsoSubspace errorInstanceSpace;
+    IsoSubspace exceptionSpace;
     IsoSubspace executableToCodeBlockEdgeSpace;
     IsoSubspace functionExecutableSpace;
     IsoSubspace indirectEvalExecutableSpace;
@@ -353,6 +355,8 @@ public:
     IsoSubspace weakSetSpace;
     IsoSubspace weakMapSpace;
     
+    IsoCellSet errorInstancesWithFinalizers;
+    IsoCellSet exceptionsWithFinalizers;
     IsoCellSet executableToCodeBlockEdgesWithConstraints;
     IsoCellSet executableToCodeBlockEdgesWithFinalizers;
     IsoCellSet inferredTypesWithFinalizers;