[inspector] cache stack frame for call sites

src/isolate.cc

Index: src/isolate.cc
diff --git a/src/isolate.cc b/src/isolate.cc
index 155e404d7b46b62d78eb9cd51e3e12617b6432d1..9e253611ef055be4c52496a90b261f06f9d5ea0b 100644
--- a/src/isolate.cc
+++ b/src/isolate.cc
@@ -625,6 +625,15 @@ class CaptureStackTraceHelper {
 
   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) {
@@ -657,6 +666,11 @@ class CaptureStackTraceHelper {
     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;
   }
 
@@ -691,6 +705,33 @@ class CaptureStackTraceHelper {
  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_;
 };