Changeset 228366 in webkit

Timestamp:

Feb 10, 2018 3:49:54 PM (6 years ago)

Author:

fpizlo@apple.com

Message:

Don't waste memory for error.stack
​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:

Location:

trunk

Files:

• JSTests/ChangeLog (modified) (1 diff)
• JSTests/stress/gc-error-stack.js (added)
• JSTests/stress/no-gc-error-stack.js (added)
• Source/JavaScriptCore/ChangeLog (modified) (1 diff)
• Source/JavaScriptCore/heap/Heap.cpp (modified) (1 diff)
• Source/JavaScriptCore/runtime/ErrorInstance.cpp (modified) (3 diffs)
• Source/JavaScriptCore/runtime/ErrorInstance.h (modified) (3 diffs)
• Source/JavaScriptCore/runtime/Exception.cpp (modified) (3 diffs)
• Source/JavaScriptCore/runtime/Exception.h (modified) (3 diffs)
• Source/JavaScriptCore/runtime/StackFrame.cpp (modified) (2 diffs)
• Source/JavaScriptCore/runtime/StackFrame.h (modified) (2 diffs)
• Source/JavaScriptCore/runtime/VM.cpp (modified) (2 diffs)
• Source/JavaScriptCore/runtime/VM.h (modified) (2 diffs)

trunk/JSTests/ChangeLog

@@ +1 @@
+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>
 

trunk/Source/JavaScriptCore/ChangeLog

@@ +1 @@
+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>
 

trunk/Source/JavaScriptCore/heap/Heap.cpp

@@ -580 +580 @@
 void Heap::finalizeUnconditionalFinalizers()
 {
+    finalizeMarkedUnconditionalFinalizers<ErrorInstance>(vm()->errorInstancesWithFinalizers);
+    finalizeMarkedUnconditionalFinalizers<Exception>(vm()->exceptionsWithFinalizers);
     finalizeMarkedUnconditionalFinalizers<InferredType>(vm()->inferredTypesWithFinalizers);
     finalizeMarkedUnconditionalFinalizers<InferredValue>(vm()->inferredValuesWithFinalizers);

trunk/Source/JavaScriptCore/runtime/ErrorInstance.cpp

@@ -1 +1 @@
 /*
  *  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
@@ -24 +24 @@
 #include "CodeBlock.h"
 #include "InlineCallFrame.h"
+#include "IsoCellSetInlines.h"
 #include "JSScope.h"
 #include "JSCInlines.h"
@@ -234 +235 @@
     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);
 }
 

trunk/Source/JavaScriptCore/runtime/ErrorInstance.h

@@ -1 +1 @@
 /*
  *  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
@@ -27 +27 @@
 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;
@@ -69 +78 @@
     
     Vector<StackFrame>* stackTrace() { return m_stackTrace.get(); }
+    
+    void finalizeUnconditionally(VM&);
 
     bool materializeErrorInfoIfNeeded(VM&);

trunk/Source/JavaScriptCore/runtime/Exception.cpp

@@ -1 +1 @@
 /*
- * 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
@@ -28 +28 @@
 
 #include "Interpreter.h"
+#include "IsoCellSetInlines.h"
 #include "JSCInlines.h"
 
@@ -58 +59 @@
     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);
 }
 

trunk/Source/JavaScriptCore/runtime/Exception.h

@@ -1 +1 @@
 /*
- * 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
@@ -32 +32 @@
 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;
@@ -61 +67 @@
 
     ~Exception();
+    
+    void finalizeUnconditionally(VM&);
 
 private:

trunk/Source/JavaScriptCore/runtime/StackFrame.cpp

@@ -1 +1 @@
 /*
- * 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
@@ -143 +143 @@
 }
 
-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();
 }
 

trunk/Source/JavaScriptCore/runtime/StackFrame.h

@@ -1 +1 @@
 /*
- * 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
@@ -57 +57 @@
     }
     
-    void visitChildren(SlotVisitor&);
+    bool isFinalizationCandidate();
+    void finalizeUnconditionally(VM&);
 
 private:

trunk/Source/JavaScriptCore/runtime/VM.cpp

@@ -252 +252 @@
 #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)
@@ -265 +267 @@
     , weakSetSpace ISO_SUBSPACE_INIT(heap, destructibleObjectHeapCellType.get(), JSWeakSet)
     , weakMapSpace ISO_SUBSPACE_INIT(heap, destructibleObjectHeapCellType.get(), JSWeakMap)
+    , errorInstancesWithFinalizers(errorInstanceSpace)
+    , exceptionsWithFinalizers(exceptionSpace)
     , executableToCodeBlockEdgesWithConstraints(executableToCodeBlockEdgeSpace)
     , executableToCodeBlockEdgesWithFinalizers(executableToCodeBlockEdgeSpace)

trunk/Source/JavaScriptCore/runtime/VM.h

@@ -340 +340 @@
     
     IsoSubspace directEvalExecutableSpace;
+    IsoSubspace errorInstanceSpace;
+    IsoSubspace exceptionSpace;
     IsoSubspace executableToCodeBlockEdgeSpace;
     IsoSubspace functionExecutableSpace;
@@ -354 +356 @@
     IsoSubspace weakMapSpace;
     
+    IsoCellSet errorInstancesWithFinalizers;
+    IsoCellSet exceptionsWithFinalizers;
     IsoCellSet executableToCodeBlockEdgesWithConstraints;
     IsoCellSet executableToCodeBlockEdgesWithFinalizers;