[inspector] cache stack frame for call sites

src/isolate.cc

 // Copyright 2012 the V8 project authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "src/isolate.h"
 
 #include <stdlib.h>
 
 #include <fstream>  // NOLINT(readability/streams)
 #include <sstream>
@@ skipping 607 matching lines @@
 
   Handle<StackFrameInfo> NewStackFrameObject(FrameSummary& summ) {
     if (summ.IsJavaScript()) return NewStackFrameObject(summ.AsJavaScript());
     if (summ.IsWasm()) return NewStackFrameObject(summ.AsWasm());
     UNREACHABLE();
     return factory()->NewStackFrameInfo();
   }
 
   Handle<StackFrameInfo> NewStackFrameObject(
       const FrameSummary::JavaScriptFrameSummary& summ) {
+    int code_offset = summ.code_offset();
+    Handle<ByteArray> source_position_table(
+        summ.abstract_code()->source_position_table(), isolate_);
+    Handle<UnseededNumberDictionary> cache = GetCache(source_position_table);
+    int entry = cache->FindEntry(code_offset);
+    if (entry != UnseededNumberDictionary::kNotFound) {
+      return handle(StackFrameInfo::cast(cache->ValueAt(entry)), isolate_);

[Yang, 2017/04/04 19:47:06]

You would check here whether the cached StackFrameInfo has the same function
name as the one from the FrameSummary. If they are not the same, you would
discard the cache entry.

[kozy, 2017/04/04 20:48:24]

It eliminates almost all performance benefits:
console.log(42) - collecting top frame
- one in the loop takes almost the same average time,
- 100 times in a loop - almost the same.

console.trace(42) - collecting full stack of 5 frames:
- one in the loop - still 22% faster,
- 100 times in a loop - ~almost the same,
- 1000 times in a loop - ~almost the same.

Based on callgrind, v8::internal::Isolate::CaptureCurrentStackTrace spend 32% in
NewStackFrameObject with function name comparison. And only 4% without
comparison. It's probably because getting displayName from function by property
name is slow.

We still will get memory usage improvement (but I need to finish inspector-side
first).

I prefer to find a way to invalidate cache on function name changed. WDYT?

[Yang, 2017/04/04 21:15:18]

Can we land this without performance improvements first, and explore other
options later?

There are some options:
- We could use a protector cell. Whenever we perform a lookup in the
LookupIterator where we would overwrite the name or displayName property of a
JSFunction, we invalidate that cell, which disables the cache forever. We do
this for example to detect when someone changes the Array constructor so that
some fast paths are no longer correct. See
https://cs.chromium.org/chromium/src/v8/src/lookup.cc?type=cs&q=protector&l=161
- We could store the function into StackFrameInfo and do the name lookup lazily.
- We could check whether the JSFunction's map is the original map, in which case
it has not transitioned, i.e. the name and displayName properties have not been
set/overwritten. That of course would disallow caching for all functions that
have additional properties.

[kozy, 2017/04/04 23:08:12]

Yes, first + third sound good to me. I'll try to implement it, I really would
like to do it. Probably at one momemnt of time we would be able to enable
capturing full detailed stack trace when DevTools are closed and resolve another
long existing feature request from out users.

+    }
+
     Handle<StackFrameInfo> frame = factory()->NewStackFrameInfo();
     Handle<Script> script = Handle<Script>::cast(summ.script());
     if (options_ & StackTrace::kLineNumber) {
       Script::PositionInfo info;
       bool valid_pos = Script::GetPositionInfo(script, summ.SourcePosition(),
                                                &info, Script::WITH_OFFSET);
       if (valid_pos) {
         frame->set_line_number(info.line + 1);
         if (options_ & StackTrace::kColumnOffset) {
           frame->set_column_number(info.column + 1);
@@ skipping 12 matching lines @@
       frame->set_is_eval(script->compilation_type() ==
                          Script::COMPILATION_TYPE_EVAL);
     }
     if (options_ & StackTrace::kFunctionName) {
       Handle<String> name = summ.FunctionName();
       frame->set_function_name(*name);
     }
     if (options_ & StackTrace::kIsConstructor) {
       frame->set_is_constructor(summ.is_constructor());
     }
+    auto new_cache =
+        UnseededNumberDictionary::AtNumberPut(cache, code_offset, frame);
+    if (*new_cache != *cache) {
+      PutCache(source_position_table, cache);
+    }
     return frame;
   }
 
   Handle<StackFrameInfo> NewStackFrameObject(
       const FrameSummary::WasmFrameSummary& summ) {
     Handle<StackFrameInfo> info = factory()->NewStackFrameInfo();
 
     if (options_ & StackTrace::kFunctionName) {
       Handle<WasmCompiledModule> compiled_module(
           summ.wasm_instance()->compiled_module(), isolate_);
@@ skipping 14 matching lines @@
     }
     if (options_ & StackTrace::kScriptId) {
       info->set_script_id(summ.script()->id());
     }
     return info;
   }
 
  private:
   inline Factory* factory() { return isolate_->factory(); }
 
+  Handle<UnseededNumberDictionary> GetCache(
+      Handle<ByteArray> source_position_table) {
+    Handle<WeakHashTable> table_to_cache(isolate_->heap()->stack_frame_cache());
+    Object* maybe_cache = table_to_cache->Lookup(source_position_table);
+    if (maybe_cache->IsUnseededNumberDictionary()) {
+      return handle(UnseededNumberDictionary::cast(maybe_cache));
+    }
+    Handle<UnseededNumberDictionary> cache =
+        UnseededNumberDictionary::New(isolate_, 1);
+    auto new_table_to_cache =
+        WeakHashTable::Put(table_to_cache, source_position_table, cache);
+    if (*new_table_to_cache != *table_to_cache) {
+      isolate_->heap()->SetStackFrameCache(*new_table_to_cache);
+    }
+    return cache;
+  }
+
+  void PutCache(Handle<ByteArray> source_position_table,
+                Handle<UnseededNumberDictionary> cache) {
+    Handle<WeakHashTable> table_to_cache(isolate_->heap()->stack_frame_cache());
+    auto new_table_to_cache =
+        WeakHashTable::Put(table_to_cache, source_position_table, cache);
+    if (*new_table_to_cache != *table_to_cache) {
+      isolate_->heap()->SetStackFrameCache(*new_table_to_cache);
+    }
+  }
+
   Isolate* isolate_;
   StackTrace::StackTraceOptions options_;
 };
 
 Handle<JSArray> Isolate::CaptureCurrentStackTrace(
     int frame_limit, StackTrace::StackTraceOptions options) {
   DisallowJavascriptExecution no_js(this);
   CaptureStackTraceHelper helper(this, options);
 
   // Ensure no negative values.
@@ skipping 2947 matching lines @@
   // Then check whether this scope intercepts.
   if ((flag & intercept_mask_)) {
     intercepted_flags_ |= flag;
     return true;
   }
   return false;
 }
 
 }  // namespace internal
 }  // namespace v8