[stack-traces] Cache source position on StackFrameInfos.

Previously we had cached the source position information on
JSStackFrame (C++) objects and reused that between calls to
GetLineNumber() and GetColumnNumber(). The refactoring in
https://crrev.com/eed0d27c2f774b3adbc85d0a5fb30a8cf0f018a8
effectively removed that cache, while still making things
faster though.

This CL puts back the caching on the StackFrameInfo objects
by reusing the `offset` slot to store the computed source
position (as indicated by a bit in the `flags`). For promise
combinator async frames, the bit is always set and the
`offset_or_source_position` slot thus always contains the source
position (aka the `promise index` in this case). We also
added a `StackFrameInfo::ComputeLocation()` method to remove the
last remaining place where we'd peek into the StackFrameInfo from
outside stack-frame-info.{cc,h}.

Also-By: kimanh@chromium.org
Bug: chromium:1077657, v8:8742, chromium:1069425
Change-Id: I59e26a91965617163776e6cc2610b88e6925452c
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2695386
Commit-Queue: Benedikt Meurer <bmeurer@chromium.org>
Auto-Submit: Benedikt Meurer <bmeurer@chromium.org>
Reviewed-by: Yang Guo <yangguo@chromium.org>
Cr-Commit-Position: refs/heads/master@{#72752}

src/builtins/builtins-callsite.cc

@@ -98,15 +98,16 @@ BUILTIN(CallSitePrototypeGetMethodName) {
 BUILTIN(CallSitePrototypeGetPosition) {
   HandleScope scope(isolate);
   CHECK_CALLSITE(frame, "getPosition");
-  return Smi::FromInt(StackFrameInfo::GetSourcePosition(isolate, frame));
+  return Smi::FromInt(StackFrameInfo::GetSourcePosition(frame));
 }
 
 BUILTIN(CallSitePrototypeGetPromiseIndex) {
   HandleScope scope(isolate);
   CHECK_CALLSITE(frame, "getPromiseIndex");
-  int index = frame->GetPromiseCombinatorIndex();
-  if (index < 0) return ReadOnlyRoots(isolate).null_value();
-  return Smi::FromInt(index);
+  if (!frame->IsPromiseAll() && !frame->IsPromiseAny()) {
+    return ReadOnlyRoots(isolate).null_value();
+  }
+  return Smi::FromInt(StackFrameInfo::GetSourcePosition(frame));
 }
 
 BUILTIN(CallSitePrototypeGetScriptNameOrSourceURL) {

src/diagnostics/objects-printer.cc

@@ -2284,7 +2284,8 @@ void StackFrameInfo::StackFrameInfoPrint(std::ostream& os) {  // NOLINT
   os << "\n - receiver_or_instance: " << Brief(receiver_or_instance());
   os << "\n - function: " << Brief(function());
   os << "\n - code_object: " << Brief(code_object());
-  os << "\n - offset: " << offset();
+  os << "\n - code_offset_or_source_position: "
+     << code_offset_or_source_position();
   os << "\n - flags: " << flags();
   os << "\n - parameters: " << Brief(parameters());
   os << "\n";

src/execution/isolate.cc

@@ -662,7 +662,8 @@ class StackTraceBuilder {
   void AppendPromiseCombinatorFrame(Handle<JSFunction> element_function,
                                     Handle<JSFunction> combinator) {
     if (!IsVisibleInStackTrace(combinator)) return;
-    int flags = StackFrameInfo::kIsAsync;
+    int flags =
+        StackFrameInfo::kIsAsync | StackFrameInfo::kIsSourcePositionComputed;
 
     Handle<Object> receiver(combinator->native_context().promise_function(),
                             isolate_);
@@ -673,9 +674,10 @@ class StackTraceBuilder {
 
     // We store the offset of the promise into the element function's
     // hash field for element callbacks.
-    int offset = Smi::ToInt(Smi::cast(element_function->GetIdentityHash())) - 1;
+    int promise_index =
+        Smi::ToInt(Smi::cast(element_function->GetIdentityHash())) - 1;
 
-    AppendFrame(receiver, combinator, code, offset, flags, parameters);
+    AppendFrame(receiver, combinator, code, promise_index, flags, parameters);
   }
 
   void AppendJavaScriptFrame(
@@ -2172,35 +2174,7 @@ bool Isolate::ComputeLocationFromStackTrace(MessageLocation* target,
   Handle<FixedArray> stack = Handle<FixedArray>::cast(property);
   for (int i = 0; i < stack->length(); i++) {
     Handle<StackFrameInfo> frame(StackFrameInfo::cast(stack->get(i)), this);
-    if (frame->IsWasm()) {
-      auto offset = StackFrameInfo::GetSourcePosition(this, frame);
-      Handle<Script> script(frame->GetWasmInstance().module_object().script(),
-                            this);
-      *target = MessageLocation(script, offset, offset + 1);
-      return true;
-    }
-
-    Handle<JSFunction> fun = handle(JSFunction::cast(frame->function()), this);
-    if (!fun->shared().IsSubjectToDebugging()) continue;
-
-    Object script = fun->shared().script();
-    if (script.IsScript() &&
-        !(Script::cast(script).source().IsUndefined(this))) {
-      Handle<SharedFunctionInfo> shared = handle(fun->shared(), this);
-
-      AbstractCode abstract_code = AbstractCode::cast(frame->code_object());
-      const int code_offset = frame->offset();
-      Handle<Script> casted_script(Script::cast(script), this);
-      if (shared->HasBytecodeArray() &&
-          shared->GetBytecodeArray(this).HasSourcePositionTable()) {
-        int pos = abstract_code.SourcePosition(code_offset);
-        *target = MessageLocation(casted_script, pos, pos + 1, shared);
-      } else {
-        *target = MessageLocation(casted_script, shared, code_offset);
-      }
-
-      return true;
-    }
+    if (StackFrameInfo::ComputeLocation(frame, target)) return true;
   }
   return false;
 }

src/heap/factory.cc

@@ -2940,14 +2940,14 @@ Handle<BreakPoint> Factory::NewBreakPoint(int id, Handle<String> condition) {
 
 Handle<StackFrameInfo> Factory::NewStackFrameInfo(
     Handle<Object> receiver_or_instance, Handle<Object> function,
-    Handle<HeapObject> code_object, int offset, int flags,
-    Handle<FixedArray> parameters) {
+    Handle<HeapObject> code_object, int code_offset_or_source_position,
+    int flags, Handle<FixedArray> parameters) {
   Handle<StackFrameInfo> info =
       Handle<StackFrameInfo>::cast(NewStruct(STACK_FRAME_INFO_TYPE));
   info->set_receiver_or_instance(*receiver_or_instance);
   info->set_function(*function);
   info->set_code_object(*code_object);
-  info->set_offset(offset);
+  info->set_code_offset_or_source_position(code_offset_or_source_position);
   info->set_flags(flags);
   info->set_parameters(*parameters);
   return info;

src/heap/factory.h

@@ -371,7 +371,8 @@ class V8_EXPORT_PRIVATE Factory : public FactoryBase<Factory> {
   Handle<StackFrameInfo> NewStackFrameInfo(Handle<Object> receiver_or_instance,
                                            Handle<Object> function,
                                            Handle<HeapObject> code_object,
-                                           int offset, int flags,
+                                           int code_offset_or_source_position,
+                                           int flags,
                                            Handle<FixedArray> parameters);
 
   // Allocate various microtasks.

src/objects/stack-frame-info.cc

@@ -4,6 +4,7 @@
 
 #include "src/objects/stack-frame-info.h"
 
+#include "src/objects/shared-function-info.h"
 #include "src/objects/stack-frame-info-inl.h"
 #include "src/strings/string-builder-inl.h"
 
@@ -57,7 +58,7 @@ int StackFrameInfo::GetLineNumber(Handle<StackFrameInfo> info) {
   }
   Handle<Script> script;
   if (GetScript(isolate, info).ToHandle(&script)) {
-    int position = GetSourcePosition(isolate, info);
+    int position = GetSourcePosition(info);
     return Script::GetLineNumber(script, position) + 1;
   }
   return Message::kNoLineNumberInfo;
@@ -66,7 +67,7 @@ int StackFrameInfo::GetLineNumber(Handle<StackFrameInfo> info) {
 // static
 int StackFrameInfo::GetColumnNumber(Handle<StackFrameInfo> info) {
   Isolate* isolate = info->GetIsolate();
-  int position = GetSourcePosition(isolate, info);
+  int position = GetSourcePosition(info);
   if (info->IsWasm() && !info->IsAsmJsWasm()) {
     return position + 1;
   }
@@ -402,10 +403,6 @@ Handle<Object> StackFrameInfo::GetTypeName(Handle<StackFrameInfo> info) {
   return JSReceiver::GetConstructorName(receiver);
 }
 
-int StackFrameInfo::GetPromiseCombinatorIndex() const {
-  return (IsPromiseAll() || IsPromiseAny()) ? offset() : kUnknown;
-}
-
 uint32_t StackFrameInfo::GetWasmFunctionIndex() const {
   DCHECK(IsWasm());
   return Smi::ToInt(Smi::cast(function()));
@@ -417,24 +414,62 @@ WasmInstanceObject StackFrameInfo::GetWasmInstance() const {
 }
 
 // static
-int StackFrameInfo::GetSourcePosition(Isolate* isolate,
-                                      Handle<StackFrameInfo> info) {
-  int offset = info->offset();
+int StackFrameInfo::GetSourcePosition(Handle<StackFrameInfo> info) {
+  if (info->flags() & kIsSourcePositionComputed) {
+    return info->code_offset_or_source_position();
+  }
+  DCHECK(!info->IsPromiseAll());
+  DCHECK(!info->IsPromiseAny());
+  int source_position =
+      ComputeSourcePosition(info, info->code_offset_or_source_position());
+  info->set_code_offset_or_source_position(source_position);
+  info->set_flags(info->flags() | kIsSourcePositionComputed);
+  return source_position;
+}
+
+// static
+bool StackFrameInfo::ComputeLocation(Handle<StackFrameInfo> info,
+                                     MessageLocation* location) {
+  Isolate* isolate = info->GetIsolate();
+  if (info->IsWasm()) {
+    int pos = GetSourcePosition(info);
+    Handle<Script> script(info->GetWasmInstance().module_object().script(),
+                          isolate);
+    *location = MessageLocation(script, pos, pos + 1);
+    return true;
+  }
+
+  Handle<SharedFunctionInfo> shared(info->GetSharedFunctionInfo(), isolate);
+  if (!shared->IsSubjectToDebugging()) return false;
+  Handle<Script> script(Script::cast(shared->script()), isolate);
+  if (script->source().IsUndefined()) return false;
+  if (info->flags() & kIsSourcePositionComputed ||
+      (shared->HasBytecodeArray() &&
+       shared->GetBytecodeArray(isolate).HasSourcePositionTable())) {
+    int pos = GetSourcePosition(info);
+    *location = MessageLocation(script, pos, pos + 1, shared);
+  } else {
+    int code_offset = info->code_offset_or_source_position();
+    *location = MessageLocation(script, shared, code_offset);
+  }
+  return true;
+}
+
+// static
+int StackFrameInfo::ComputeSourcePosition(Handle<StackFrameInfo> info,
+                                          int offset) {
+  Isolate* isolate = info->GetIsolate();
   if (info->IsWasm()) {
     auto code_ref = Managed<wasm::GlobalWasmCodeRef>::cast(info->code_object());
-    offset = code_ref.get()->code()->GetSourcePositionBefore(offset);
+    int byte_offset = code_ref.get()->code()->GetSourcePositionBefore(offset);
     auto module = info->GetWasmInstance().module();
-    auto func_index = info->GetWasmFunctionIndex();
-    offset = wasm::GetSourcePosition(module, func_index, offset,
-                                     info->IsAsmJsAtNumberConversion());
-  } else {
-    Handle<SharedFunctionInfo> shared(info->GetSharedFunctionInfo(), isolate);
-    SharedFunctionInfo::EnsureSourcePositionsAvailable(isolate, shared);
-    offset = AbstractCode::cast(info->code_object()).SourcePosition(offset);
+    uint32_t func_index = info->GetWasmFunctionIndex();
+    return wasm::GetSourcePosition(module, func_index, byte_offset,
+                                   info->IsAsmJsAtNumberConversion());
   }
-  // A possible optimization here would be to cache the computed position in
-  // the offset slot and somehow mark the StackFrameInfo object appropriately.
-  return offset;
+  Handle<SharedFunctionInfo> shared(info->GetSharedFunctionInfo(), isolate);
+  SharedFunctionInfo::EnsureSourcePositionsAvailable(isolate, shared);
+  return AbstractCode::cast(info->code_object()).SourcePosition(offset);
 }
 
 // static
@@ -599,7 +634,7 @@ void SerializeJSStackFrame(Isolate* isolate, Handle<StackFrameInfo> frame,
       builder->AppendCString("Promise.");
       builder->AppendString(Handle<String>::cast(function_name));
       builder->AppendCString(" (index ");
-      builder->AppendInt(frame->GetPromiseCombinatorIndex());
+      builder->AppendInt(StackFrameInfo::GetSourcePosition(frame));
       builder->AppendCString(")");
       return;
     }

src/objects/stack-frame-info.h

@@ -14,6 +14,7 @@
 namespace v8 {
 namespace internal {
 
+class MessageLocation;
 class WasmInstanceObject;
 
 #include "torque-generated/src/objects/stack-frame-info-tq.inc"
@@ -61,10 +62,6 @@ class StackFrameInfo
   static Handle<Object> GetMethodName(Handle<StackFrameInfo> info);
   static Handle<Object> GetTypeName(Handle<StackFrameInfo> info);
 
-  // Returns the promise combinator input index for Promise.all()
-  // and Promise.any() async frames, kUnknown otherwise.
-  int GetPromiseCombinatorIndex() const;
-
   // These methods are only valid for Wasm and asm.js Wasm frames.
   uint32_t GetWasmFunctionIndex() const;
   WasmInstanceObject GetWasmInstance() const;
@@ -72,14 +69,22 @@ class StackFrameInfo
 
   // Returns the 0-based source position, which is the offset into the
   // Script in case of JavaScript and Asm.js, and the bytecode offset
-  // in the module in case of actual Wasm.
-  static int GetSourcePosition(Isolate* isolate, Handle<StackFrameInfo> info);
+  // in the module in case of actual Wasm. In case of async promise
+  // combinator frames, this returns the index of the promise.
+  static int GetSourcePosition(Handle<StackFrameInfo> info);
+
+  // Attempts to fill the |location| based on the |info|, and avoids
+  // triggering source position table building for JavaScript frames.
+  static bool ComputeLocation(Handle<StackFrameInfo> info,
+                              MessageLocation* location);
 
  private:
   // Bit position in the flag, from least significant bit position.
   DEFINE_TORQUE_GENERATED_STACK_FRAME_INFO_FLAGS()
   friend class StackTraceBuilder;
 
+  static int ComputeSourcePosition(Handle<StackFrameInfo> info, int offset);
+
   base::Optional<Script> GetScript() const;
   SharedFunctionInfo GetSharedFunctionInfo() const;
 

src/objects/stack-frame-info.tq

@@ -9,6 +9,9 @@ bitfield struct StackFrameInfoFlags extends uint31 {
   is_constructor: bool: 1 bit;
   is_asm_js_at_number_conversion: bool: 1 bit;
   is_async: bool: 1 bit;
+
+  // whether offset_or_source_position contains the source position.
+  is_source_position_computed: bool: 1 bit;
 }
 
 @generateCppClass
@@ -16,7 +19,7 @@ extern class StackFrameInfo extends Struct {
   receiver_or_instance: JSAny;
   function: JSFunction|Smi;
   code_object: HeapObject;
-  offset: Smi;
+  code_offset_or_source_position: Smi;
   flags: SmiTagged<StackFrameInfoFlags>;
   parameters: FixedArray;
 }

src/wasm/c-api.cc

@@ -890,7 +890,7 @@ own<Frame> CreateFrameFromInternal(i::Handle<i::FixedArray> frames, int index,
       i::StackFrameInfo::cast(frames->get(index)), isolate);
   i::Handle<i::WasmInstanceObject> instance(frame->GetWasmInstance(), isolate);
   uint32_t func_index = frame->GetWasmFunctionIndex();
-  size_t module_offset = i::StackFrameInfo::GetSourcePosition(isolate, frame);
+  size_t module_offset = i::StackFrameInfo::GetSourcePosition(frame);
   size_t func_offset = module_offset - i::wasm::GetWasmFunctionOffset(
                                            instance->module(), func_index);
   return own<Frame>(seal<Frame>(new (std::nothrow) FrameImpl(