[stack-traces] Remove StackFrameInfo.

For a long time, V8 had two distinct ways to capture and store a stack
trace, one where we'd just collect and symbolize the information for the
v8::StackTrace API (script id, name, line and colum information mostly),
and one where V8 would also memorize the closures, receivers, and
optionally the parameters of the stack frame, which we use for
Error.stack and the non-standard CallSite APIs. Those two were often out
of sync and suffered from various different issues. Eventually they were
refactored into a single captureStackTrace() bottleneck that would
produce a FrameArray.

This CL is a logical continuation of the refactorings. It repairs a
regression where we'd compute the method name (as part of the
cached StackFrameInfo) even if we don't need them (as is the case for
the inspector and any other use of the v8::StackTrace API).

Everytime a method was invoked on StackTraceFrame, it'd call into
StackTraceFrame::GetInfo(), which would lazily setup the StackFrameInfo
like this:

  1. Create a FrameArrayIterator and point it to the FrameArray at the
     index stored in the StackTraceFrame.
  2. Invoke FrameArrayIterator::Frame(), which copies the information
     from the FrameArray into a temporary JSStackFrame, AsmJsStackFrame
     or WasmStackFrame C++ object, and use the StackFrameBase virtual
     methods to transfer all information to a newly created
     StackFrameInfo object.
  3. Kill the link to the FrameArray and put a link to the
     StackFrameInfo object into the StackTraceFrame.

This caching turned out to be extremely costly, since beyond other
things, it'd always invoke JSStackFrame::GetMethodName(), which is
extremely costly (the execution time is linear in the number of
properties on the receiver and it's prototype chain). The cost was so
high that several work-arounds had been added, which would avoid
triggering the eager construction of the StackFrameInfo object (i.e.
https://crrev.com/c/2080663, https://crrev.com/c/2550504 or
https://crrev.com/c/2261736, but also https://crrev.com/c/1688927).

This CL removes the StackFrameInfo caching completely, since neither the
inspector nor Error.stack benefit from the caching at all. It's only the
first part in a series of refactorings that will significantly reduce
the complexity and overhead of the stack trace collection.

Doc: https://bit.ly/2wkbuIy
Bug: chromium:1057211, chromium:1077657, chromium:1069425, v8:8742
Bug: chromium:1127391, chromium:1098530, chromium:981541
Change-Id: I8edb8ff48b620eb3043ae51ab4ea27146ef0a5a2
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2689185
Commit-Queue: Benedikt Meurer <bmeurer@chromium.org>
Commit-Queue: Yang Guo <yangguo@chromium.org>
Auto-Submit: Benedikt Meurer <bmeurer@chromium.org>
Reviewed-by: Simon Zünd <szuend@chromium.org>
Reviewed-by: Yang Guo <yangguo@chromium.org>
Cr-Commit-Position: refs/heads/master@{#72647}

src/diagnostics/objects-printer.cc

@@ -2287,21 +2287,7 @@ void ScopeInfo::ScopeInfoPrint(std::ostream& os) {  // NOLINT
 void StackTraceFrame::StackTraceFramePrint(std::ostream& os) {  // NOLINT
   PrintHeader(os, "StackTraceFrame");
   os << "\n - frame_index: " << frame_index();
-  os << "\n - frame_info: " << Brief(frame_info());
-  os << "\n";
-}
-
-void StackFrameInfo::StackFrameInfoPrint(std::ostream& os) {  // NOLINT
-  PrintHeader(os, "StackFrame");
-  os << "\n - line_number: " << line_number();
-  os << "\n - column_number: " << column_number();
-  os << "\n - script_id: " << script_id();
-  os << "\n - script_name: " << Brief(script_name());
-  os << "\n - script_name_or_source_url: "
-     << Brief(script_name_or_source_url());
-  os << "\n - function_name: " << Brief(function_name());
-  os << "\n - is_eval: " << (is_eval() ? "true" : "false");
-  os << "\n - is_constructor: " << (is_constructor() ? "true" : "false");
+  os << "\n - frame_array: " << Brief(frame_array());
   os << "\n";
 }
 

src/execution/isolate.cc

@@ -710,11 +710,7 @@ class FrameArrayBuilder {
     Handle<JSFunction> function = summary.function();
     if (IsStrictFrame(function)) flags |= FrameArray::kIsStrict;
     if (is_constructor) flags |= FrameArray::kIsConstructor;
-
-    Handle<FixedArray> parameters = isolate_->factory()->empty_fixed_array();
-    if (V8_UNLIKELY(FLAG_detailed_error_stack_trace)) {
-      parameters = summary.parameters();
-    }
+    Handle<FixedArray> parameters = summary.parameters();
 
     elements_ = FrameArray::AppendJSFrame(
         elements_, TheHoleToUndefined(isolate_, summary.receiver()), function,
@@ -893,6 +889,8 @@ bool GetStackTraceLimit(Isolate* isolate, int* result) {
 
 bool NoExtension(const v8::FunctionCallbackInfo<v8::Value>&) { return false; }
 
+namespace {
+
 bool IsBuiltinFunction(Isolate* isolate, HeapObject object,
                        Builtins::Name builtin_index) {
   if (!object.IsJSFunction()) return false;
@@ -1013,8 +1011,6 @@ void CaptureAsyncStackTrace(Isolate* isolate, Handle<JSPromise> promise,
   }
 }
 
-namespace {
-
 struct CaptureStackTraceOptions {
   int limit;
   // 'filter_mode' and 'skip_mode' are somewhat orthogonal. 'filter_mode'

src/heap/factory.cc

@@ -2941,96 +2941,10 @@ Handle<StackTraceFrame> Factory::NewStackTraceFrame(
       NewStruct(STACK_TRACE_FRAME_TYPE, AllocationType::kYoung));
   frame->set_frame_array(*frame_array);
   frame->set_frame_index(index);
-  frame->set_frame_info(*undefined_value());
 
   return frame;
 }
 
-Handle<StackFrameInfo> Factory::NewStackFrameInfo(
-    Handle<FrameArray> frame_array, int index) {
-  FrameArrayIterator it(isolate(), frame_array, index);
-  DCHECK(it.HasFrame());
-
-  const bool is_wasm = frame_array->IsAnyWasmFrame(index);
-  StackFrameBase* frame = it.Frame();
-
-  int line = frame->GetLineNumber();
-  int column = frame->GetColumnNumber();
-  int wasm_function_index = frame->GetWasmFunctionIndex();
-
-  const int script_id = frame->GetScriptId();
-
-  Handle<Object> script_name = frame->GetFileName();
-  Handle<Object> script_or_url = frame->GetScriptNameOrSourceUrl();
-
-  // TODO(szuend): Adjust this, once it is decided what name to use in both
-  //               "simple" and "detailed" stack traces. This code is for
-  //               backwards compatibility to fullfill test expectations.
-  Handle<PrimitiveHeapObject> function_name = frame->GetFunctionName();
-  bool is_user_java_script = false;
-  if (!is_wasm) {
-    Handle<Object> function = frame->GetFunction();
-    if (function->IsJSFunction()) {
-      Handle<JSFunction> fun = Handle<JSFunction>::cast(function);
-
-      is_user_java_script = fun->shared().IsUserJavaScript();
-    }
-  }
-
-  Handle<PrimitiveHeapObject> method_name = undefined_value();
-  Handle<PrimitiveHeapObject> type_name = undefined_value();
-  Handle<PrimitiveHeapObject> eval_origin = frame->GetEvalOrigin();
-  Handle<PrimitiveHeapObject> wasm_module_name = frame->GetWasmModuleName();
-  Handle<HeapObject> wasm_instance = frame->GetWasmInstance();
-
-  // MethodName and TypeName are expensive to look up, so they are only
-  // included when they are strictly needed by the stack trace
-  // serialization code.
-  // Note: The {is_method_call} predicate needs to be kept in sync with
-  //       the corresponding predicate in the stack trace serialization code
-  //       in stack-frame-info.cc.
-  const bool is_toplevel = frame->IsToplevel();
-  const bool is_constructor = frame->IsConstructor();
-  const bool is_method_call = !(is_toplevel || is_constructor);
-  if (is_method_call) {
-    method_name = frame->GetMethodName();
-    type_name = frame->GetTypeName();
-  }
-
-  Handle<StackFrameInfo> info = Handle<StackFrameInfo>::cast(
-      NewStruct(STACK_FRAME_INFO_TYPE, AllocationType::kYoung));
-
-  DisallowGarbageCollection no_gc;
-
-  info->set_flag(0);
-  info->set_is_wasm(is_wasm);
-  info->set_is_asmjs_wasm(frame_array->IsAsmJsWasmFrame(index));
-  info->set_is_user_java_script(is_user_java_script);
-  info->set_line_number(line);
-  info->set_column_number(column);
-  info->set_wasm_function_index(wasm_function_index);
-  info->set_script_id(script_id);
-
-  info->set_script_name(*script_name);
-  info->set_script_name_or_source_url(*script_or_url);
-  info->set_function_name(*function_name);
-  info->set_method_name(*method_name);
-  info->set_type_name(*type_name);
-  info->set_eval_origin(*eval_origin);
-  info->set_wasm_module_name(*wasm_module_name);
-  info->set_wasm_instance(*wasm_instance);
-
-  info->set_is_eval(frame->IsEval());
-  info->set_is_constructor(is_constructor);
-  info->set_is_toplevel(is_toplevel);
-  info->set_is_async(frame->IsAsync());
-  info->set_is_promise_all(frame->IsPromiseAll());
-  info->set_is_promise_any(frame->IsPromiseAny());
-  info->set_promise_combinator_index(frame->GetPromiseIndex());
-
-  return info;
-}
-
 Handle<JSObject> Factory::NewArgumentsObject(Handle<JSFunction> callee,
                                              int length) {
   bool strict_mode_callee = is_strict(callee->shared().language_mode()) ||

src/heap/factory.h

@@ -59,7 +59,6 @@ class PromiseResolveThenableJobTask;
 class RegExpMatchInfo;
 class ScriptContextTable;
 class SourceTextModule;
-class StackFrameInfo;
 class StackTraceFrame;
 class StringSet;
 class StoreHandler;
@@ -368,8 +367,6 @@ class V8_EXPORT_PRIVATE Factory : public FactoryBase<Factory> {
   Handle<BreakPoint> NewBreakPoint(int id, Handle<String> condition);
   Handle<StackTraceFrame> NewStackTraceFrame(Handle<FrameArray> frame_array,
                                              int index);
-  Handle<StackFrameInfo> NewStackFrameInfo(Handle<FrameArray> frame_array,
-                                           int index);
 
   // Allocate various microtasks.
   Handle<CallableTask> NewCallableTask(Handle<JSReceiver> callable,

src/objects/objects-definitions.h

@@ -146,7 +146,6 @@ namespace internal {
   V(_, SCRIPT_TYPE, Script, script)                                           \
   V(_, SOURCE_TEXT_MODULE_INFO_ENTRY_TYPE, SourceTextModuleInfoEntry,         \
     module_info_entry)                                                        \
-  V(_, STACK_FRAME_INFO_TYPE, StackFrameInfo, stack_frame_info)               \
   V(_, STACK_TRACE_FRAME_TYPE, StackTraceFrame, stack_trace_frame)            \
   V(_, TEMPLATE_OBJECT_DESCRIPTION_TYPE, TemplateObjectDescription,           \
     template_object_description)                                              \

src/objects/objects.h

@@ -166,7 +166,6 @@
 //       - BreakPoint
 //       - BreakPointInfo
 //       - CachedTemplateObject
-//       - StackFrameInfo
 //       - StackTraceFrame
 //       - CodeCache
 //       - PropertyDescriptorObject

src/objects/stack-frame-info-inl.h

@@ -20,23 +20,6 @@ namespace internal {
 
 #include "torque-generated/src/objects/stack-frame-info-tq-inl.inc"
 
-TQ_OBJECT_CONSTRUCTORS_IMPL(StackFrameInfo)
-
-NEVER_READ_ONLY_SPACE_IMPL(StackFrameInfo)
-
-SMI_ACCESSORS_CHECKED(StackFrameInfo, function_offset,
-                      kPromiseCombinatorIndexOffset, is_wasm())
-BOOL_ACCESSORS(StackFrameInfo, flag, is_eval, IsEvalBit::kShift)
-BOOL_ACCESSORS(StackFrameInfo, flag, is_constructor, IsConstructorBit::kShift)
-BOOL_ACCESSORS(StackFrameInfo, flag, is_wasm, IsWasmBit::kShift)
-BOOL_ACCESSORS(StackFrameInfo, flag, is_asmjs_wasm, IsAsmJsWasmBit::kShift)
-BOOL_ACCESSORS(StackFrameInfo, flag, is_user_java_script,
-               IsUserJavaScriptBit::kShift)
-BOOL_ACCESSORS(StackFrameInfo, flag, is_toplevel, IsToplevelBit::kShift)
-BOOL_ACCESSORS(StackFrameInfo, flag, is_async, IsAsyncBit::kShift)
-BOOL_ACCESSORS(StackFrameInfo, flag, is_promise_all, IsPromiseAllBit::kShift)
-BOOL_ACCESSORS(StackFrameInfo, flag, is_promise_any, IsPromiseAnyBit::kShift)
-
 TQ_OBJECT_CONSTRUCTORS_IMPL(StackTraceFrame)
 NEVER_READ_ONLY_SPACE_IMPL(StackTraceFrame)
 

src/objects/stack-frame-info.h

@@ -19,37 +19,6 @@ class WasmInstanceObject;
 
 #include "torque-generated/src/objects/stack-frame-info-tq.inc"
 
-class StackFrameInfo
-    : public TorqueGeneratedStackFrameInfo<StackFrameInfo, Struct> {
- public:
-  NEVER_READ_ONLY_SPACE
-  // Wasm frames only: function_offset instead of promise_combinator_index.
-  DECL_INT_ACCESSORS(function_offset)
-  DECL_BOOLEAN_ACCESSORS(is_eval)
-  DECL_BOOLEAN_ACCESSORS(is_constructor)
-  DECL_BOOLEAN_ACCESSORS(is_wasm)
-  DECL_BOOLEAN_ACCESSORS(is_asmjs_wasm)
-  DECL_BOOLEAN_ACCESSORS(is_user_java_script)
-  DECL_BOOLEAN_ACCESSORS(is_toplevel)
-  DECL_BOOLEAN_ACCESSORS(is_async)
-  DECL_BOOLEAN_ACCESSORS(is_promise_all)
-  DECL_BOOLEAN_ACCESSORS(is_promise_any)
-
-  // Dispatched behavior.
-  DECL_PRINTER(StackFrameInfo)
-
- private:
-  // Bit position in the flag, from least significant bit position.
-  DEFINE_TORQUE_GENERATED_STACK_FRAME_INFO_FLAGS()
-
-  TQ_OBJECT_CONSTRUCTORS(StackFrameInfo)
-};
-
-// This class is used to lazily initialize a StackFrameInfo object from
-// a FrameArray plus an index.
-// The first time any of the Get* or Is* methods is called, a
-// StackFrameInfo object is allocated and all necessary information
-// retrieved.
 class StackTraceFrame
     : public TorqueGeneratedStackTraceFrame<StackTraceFrame, Struct> {
  public:
@@ -79,6 +48,7 @@ class StackTraceFrame
 
   static bool IsEval(Handle<StackTraceFrame> frame);
   static bool IsConstructor(Handle<StackTraceFrame> frame);
+  static bool IsMethodCall(Handle<StackTraceFrame> frame);
   static bool IsWasm(Handle<StackTraceFrame> frame);
   static bool IsAsmJsWasm(Handle<StackTraceFrame> frame);
   static bool IsUserJavaScript(Handle<StackTraceFrame> frame);
@@ -88,9 +58,6 @@ class StackTraceFrame
   static bool IsPromiseAny(Handle<StackTraceFrame> frame);
 
  private:
-  static Handle<StackFrameInfo> GetFrameInfo(Handle<StackTraceFrame> frame);
-  static void InitializeFrameInfo(Handle<StackTraceFrame> frame);
-
   TQ_OBJECT_CONSTRUCTORS(StackTraceFrame)
 };
 

src/objects/stack-frame-info.tq

@@ -2,39 +2,8 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-bitfield struct StackFrameInfoFlags extends uint31 {
-  is_eval: bool: 1 bit;
-  is_constructor: bool: 1 bit;
-  is_wasm: bool: 1 bit;
-  is_asm_js_wasm: bool: 1 bit;
-  is_user_java_script: bool: 1 bit;
-  is_toplevel: bool: 1 bit;
-  is_async: bool: 1 bit;
-  is_promise_all: bool: 1 bit;
-  is_promise_any: bool: 1 bit;
-}
-
-@generateCppClass
-extern class StackFrameInfo extends Struct {
-  line_number: Smi;
-  column_number: Smi;
-  promise_combinator_index: Smi;
-  script_id: Smi;
-  wasm_function_index: Smi;
-  script_name: Object;
-  script_name_or_source_url: Object;
-  function_name: String|Null|Undefined;
-  method_name: String|Null|Undefined;
-  type_name: String|Null|Undefined;
-  eval_origin: String|Null|Undefined;
-  wasm_module_name: String|Null|Undefined;
-  wasm_instance: WasmInstanceObject|Null|Undefined;
-  flag: SmiTagged<StackFrameInfoFlags>;
-}
-
 @generateCppClass
 extern class StackTraceFrame extends Struct {
   frame_array: FrameArray|Undefined;
   frame_index: Smi;
-  frame_info: StackFrameInfo|Undefined;
 }