Minimize the amount of memcpy done for allocating Error stacks.
authormark.lam@apple.com <mark.lam@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sat, 11 Jun 2016 19:58:07 +0000 (19:58 +0000)
committermark.lam@apple.com <mark.lam@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sat, 11 Jun 2016 19:58:07 +0000 (19:58 +0000)
https://bugs.webkit.org/show_bug.cgi?id=158664

Reviewed by Darin Adler.

Currently, Vector<StackFrame> are being copied around multiple times in the
process of creating Error stacks.

This patch avoids this unnecessary copying by:
1. Sizing the StackFrame vector correctly to begin with, and skipping
   undesirable top frames before filling in the vector.
2. Using perfect forwarding or passing by reference to pass the vector data around
   instead of copying the vectors.
3. Changing the Exception object to take a Vector<StackFrame> instead of a
   RefCountedArray<StackFrame>.

This patch has passed the JSC and layout tests.  Benchmarks show that perf is
neutral.

* API/tests/testapi.mm:
(testObjectiveCAPI):
* inspector/ScriptCallStackFactory.cpp:
(Inspector::createScriptCallStackFromException):
* interpreter/Interpreter.cpp:
(JSC::GetStackTraceFunctor::GetStackTraceFunctor):
(JSC::GetStackTraceFunctor::operator()):
(JSC::Interpreter::getStackTrace):
(JSC::Interpreter::stackTraceAsString):
(JSC::findExceptionHandler):
* interpreter/Interpreter.h:
* runtime/Error.cpp:
(JSC::addErrorInfoAndGetBytecodeOffset):
* runtime/Exception.cpp:
(JSC::Exception::finishCreation):
* runtime/Exception.h:
(JSC::Exception::valueOffset):
(JSC::Exception::value):
(JSC::Exception::stack):
(JSC::Exception::didNotifyInspectorOfThrow):
(JSC::Exception::setDidNotifyInspectorOfThrow):

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

Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/inspector/ScriptCallStackFactory.cpp
Source/JavaScriptCore/interpreter/Interpreter.cpp
Source/JavaScriptCore/interpreter/Interpreter.h
Source/JavaScriptCore/runtime/Error.cpp
Source/JavaScriptCore/runtime/Exception.cpp
Source/JavaScriptCore/runtime/Exception.h

index 2969d0d..6d32033 100644 (file)
@@ -1,5 +1,48 @@
 2016-06-11  Mark Lam  <mark.lam@apple.com>
 
+        Minimize the amount of memcpy done for allocating Error stacks.
+        https://bugs.webkit.org/show_bug.cgi?id=158664
+
+        Reviewed by Darin Adler.
+
+        Currently, Vector<StackFrame> are being copied around multiple times in the
+        process of creating Error stacks.
+
+        This patch avoids this unnecessary copying by:
+        1. Sizing the StackFrame vector correctly to begin with, and skipping
+           undesirable top frames before filling in the vector.
+        2. Using perfect forwarding or passing by reference to pass the vector data around
+           instead of copying the vectors.
+        3. Changing the Exception object to take a Vector<StackFrame> instead of a
+           RefCountedArray<StackFrame>.
+
+        This patch has passed the JSC and layout tests.  Benchmarks show that perf is
+        neutral.
+
+        * API/tests/testapi.mm:
+        (testObjectiveCAPI):
+        * inspector/ScriptCallStackFactory.cpp:
+        (Inspector::createScriptCallStackFromException):
+        * interpreter/Interpreter.cpp:
+        (JSC::GetStackTraceFunctor::GetStackTraceFunctor):
+        (JSC::GetStackTraceFunctor::operator()):
+        (JSC::Interpreter::getStackTrace):
+        (JSC::Interpreter::stackTraceAsString):
+        (JSC::findExceptionHandler):
+        * interpreter/Interpreter.h:
+        * runtime/Error.cpp:
+        (JSC::addErrorInfoAndGetBytecodeOffset):
+        * runtime/Exception.cpp:
+        (JSC::Exception::finishCreation):
+        * runtime/Exception.h:
+        (JSC::Exception::valueOffset):
+        (JSC::Exception::value):
+        (JSC::Exception::stack):
+        (JSC::Exception::didNotifyInspectorOfThrow):
+        (JSC::Exception::setDidNotifyInspectorOfThrow):
+
+2016-06-11  Mark Lam  <mark.lam@apple.com>
+
         Tests that overflows the stack should not be run with the sampling profiler.
         https://bugs.webkit.org/show_bug.cgi?id=158663
 
index b639d11..bd4c1c5 100644 (file)
@@ -137,7 +137,7 @@ static void extractSourceInformationFromException(JSC::ExecState* exec, JSObject
 Ref<ScriptCallStack> createScriptCallStackFromException(JSC::ExecState* exec, JSC::Exception* exception, size_t maxStackSize)
 {
     Vector<ScriptCallFrame> frames;
-    RefCountedArray<StackFrame> stackTrace = exception->stack();
+    auto& stackTrace = exception->stack();
     VM& vm = exec->vm();
     for (size_t i = 0; i < stackTrace.size() && i < maxStackSize; i++) {
         unsigned line;
index 0418db6..c111d00 100644 (file)
@@ -498,29 +498,35 @@ static inline bool isWebAssemblyExecutable(ExecutableBase* executable)
 
 class GetStackTraceFunctor {
 public:
-    GetStackTraceFunctor(VM& vm, Vector<StackFrame>& results, size_t remainingCapacity)
+    GetStackTraceFunctor(VM& vm, Vector<StackFrame>& results, size_t framesToSkip, size_t capacity)
         : m_vm(vm)
         , m_results(results)
-        , m_remainingCapacityForFrameCapture(remainingCapacity)
+        , m_framesToSkip(framesToSkip)
+        , m_remainingCapacityForFrameCapture(capacity)
     {
+        m_results.reserveInitialCapacity(capacity);
     }
 
     StackVisitor::Status operator()(StackVisitor& visitor) const
     {
-        VM& vm = m_vm;
+        if (m_framesToSkip > 0) {
+            m_framesToSkip--;
+            return StackVisitor::Continue;
+        }
+
         if (m_remainingCapacityForFrameCapture) {
             if (visitor->isJSFrame()
                 && !isWebAssemblyExecutable(visitor->codeBlock()->ownerExecutable())
                 && !visitor->codeBlock()->unlinkedCodeBlock()->isBuiltinFunction()) {
                 StackFrame s = {
-                    Strong<JSObject>(vm, visitor->callee()),
-                    Strong<CodeBlock>(vm, visitor->codeBlock()),
+                    Strong<JSObject>(m_vm, visitor->callee()),
+                    Strong<CodeBlock>(m_vm, visitor->codeBlock()),
                     visitor->bytecodeOffset()
                 };
                 m_results.append(s);
             } else {
                 StackFrame s = {
-                    Strong<JSObject>(vm, visitor->callee()),
+                    Strong<JSObject>(m_vm, visitor->callee()),
                     Strong<CodeBlock>(),
                     0 // unused value because codeBlock is null.
                 };
@@ -536,31 +542,43 @@ public:
 private:
     VM& m_vm;
     Vector<StackFrame>& m_results;
+    mutable size_t m_framesToSkip;
     mutable size_t m_remainingCapacityForFrameCapture;
 };
 
-void Interpreter::getStackTrace(Vector<StackFrame>& results, size_t maxStackSize)
+void Interpreter::getStackTrace(Vector<StackFrame>& results, size_t framesToSkip, size_t maxStackSize)
 {
     VM& vm = m_vm;
     CallFrame* callFrame = vm.topCallFrame;
     if (!callFrame)
         return;
 
-    GetStackTraceFunctor functor(vm, results, maxStackSize);
+    size_t framesCount = 0;
+    callFrame->iterate([&] (StackVisitor&) -> StackVisitor::Status {
+        framesCount++;
+        return StackVisitor::Continue;
+    });
+    if (framesCount <= framesToSkip)
+        return;
+
+    framesCount -= framesToSkip;
+    framesCount = std::min(maxStackSize, framesCount);
+
+    GetStackTraceFunctor functor(vm, results, framesToSkip, framesCount);
     callFrame->iterate(functor);
+    ASSERT(results.size() == results.capacity());
 }
 
-JSString* Interpreter::stackTraceAsString(ExecState* exec, Vector<StackFrame> stackTrace)
+JSString* Interpreter::stackTraceAsString(VM& vm, const Vector<StackFrame>& stackTrace)
 {
     // FIXME: JSStringJoiner could be more efficient than StringBuilder here.
     StringBuilder builder;
-    VM& vm = exec->vm();
     for (unsigned i = 0; i < stackTrace.size(); i++) {
         builder.append(String(stackTrace[i].toString(vm)));
         if (i != stackTrace.size() - 1)
             builder.append('\n');
     }
-    return jsString(&exec->vm(), builder.toString());
+    return jsString(&vm, builder.toString());
 }
 
 ALWAYS_INLINE static HandlerInfo* findExceptionHandler(StackVisitor& visitor, CodeBlock* codeBlock, CodeBlock::RequiredHandler requiredHandler)
index da133ee..6d5a41d 100644 (file)
@@ -217,7 +217,7 @@ namespace JSC {
         NEVER_INLINE HandlerInfo* unwind(VM&, CallFrame*&, Exception*, UnwindStart);
         void notifyDebuggerOfExceptionToBeThrown(CallFrame*, Exception*);
         NEVER_INLINE void debug(CallFrame*, DebugHookID);
-        JSString* stackTraceAsString(ExecState*, Vector<StackFrame>);
+        static JSString* stackTraceAsString(VM&, const Vector<StackFrame>&);
 
         static EncodedJSValue JSC_HOST_CALL constructWithErrorConstructor(ExecState*);
         static EncodedJSValue JSC_HOST_CALL callErrorConstructor(ExecState*);
@@ -226,7 +226,7 @@ namespace JSC {
 
         JS_EXPORT_PRIVATE void dumpCallFrame(CallFrame*);
 
-        void getStackTrace(Vector<StackFrame>& results, size_t maxStackSize = std::numeric_limits<size_t>::max());
+        void getStackTrace(Vector<StackFrame>& results, size_t framesToSkip = 0, size_t maxStackSize = std::numeric_limits<size_t>::max());
 
     private:
         enum ExecutionFlag { Normal, InitializeAndReturn };
index f267124..b330791 100644 (file)
@@ -143,7 +143,8 @@ bool addErrorInfoAndGetBytecodeOffset(ExecState* exec, VM& vm, JSObject* obj, bo
 {
     Vector<StackFrame> stackTrace = Vector<StackFrame>();
 
-    vm.interpreter->getStackTrace(stackTrace);
+    size_t framesToSkip = useCurrentFrame ? 0 : 1;
+    vm.interpreter->getStackTrace(stackTrace, framesToSkip);
     if (!stackTrace.isEmpty()) {
 
         ASSERT(exec == vm.topCallFrame || exec == exec->lexicalGlobalObject()->globalExec() || exec == exec->vmEntryGlobalObject()->globalExec());
@@ -173,9 +174,7 @@ bool addErrorInfoAndGetBytecodeOffset(ExecState* exec, VM& vm, JSObject* obj, bo
         if (!frameSourceURL.isEmpty())
             obj->putDirect(vm, vm.propertyNames->sourceURL, jsString(&vm, frameSourceURL), ReadOnly | DontDelete);
 
-        if (!useCurrentFrame)
-            stackTrace.remove(0);
-        obj->putDirect(vm, vm.propertyNames->stack, vm.interpreter->stackTraceAsString(vm.topCallFrame, stackTrace), DontEnum);
+        obj->putDirect(vm, vm.propertyNames->stack, Interpreter::stackTraceAsString(vm, stackTrace), DontEnum);
 
         return true;
     }
index 4f5fcb9..051d762 100644 (file)
@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2015 Apple Inc. All rights reserved.
+ * Copyright (C) 2015-2016 Apple Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
@@ -77,7 +77,7 @@ void Exception::finishCreation(VM& vm, JSValue thrownValue, StackCaptureAction a
     Vector<StackFrame> stackTrace;
     if (action == StackCaptureAction::CaptureStack)
         vm.interpreter->getStackTrace(stackTrace);
-    m_stack = RefCountedArray<StackFrame>(stackTrace);
+    m_stack = WTFMove(stackTrace);
 }
 
 } // namespace JSC
index 491cb49..a73c680 100644 (file)
@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2015 Apple Inc. All rights reserved.
+ * Copyright (C) 2015-2016 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,7 +27,7 @@
 #define Exception_h
 
 #include "Interpreter.h"
-#include <wtf/RefCountedArray.h>
+#include <wtf/Vector.h>
 
 namespace JSC {
     
@@ -57,7 +57,7 @@ public:
     }
 
     JSValue value() const { return m_value.get(); }
-    const RefCountedArray<StackFrame>& stack() const { return m_stack; }
+    const Vector<StackFrame>& stack() const { return m_stack; }
 
     bool didNotifyInspectorOfThrow() const { return m_didNotifyInspectorOfThrow; }
     void setDidNotifyInspectorOfThrow() { m_didNotifyInspectorOfThrow = true; }
@@ -69,7 +69,7 @@ private:
     void finishCreation(VM&, JSValue thrownValue, StackCaptureAction);
 
     WriteBarrier<Unknown> m_value;
-    RefCountedArray<StackFrame> m_stack;
+    Vector<StackFrame> m_stack;
     bool m_didNotifyInspectorOfThrow { false };
 
     friend class LLIntOffsetsExtractor;