[debugger] Refactor LiveEdit function info collection

src/debug/liveedit.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/debug/liveedit.h"
 
 #include "src/ast/scopeinfo.h"
 #include "src/ast/scopes.h"
 #include "src/code-stubs.h"
 #include "src/compilation-cache.h"
@@ skipping 702 matching lines @@
     FunctionInfoWrapper info = FunctionInfoWrapper::cast(
         *JSReceiver::GetElement(isolate(), result_, current_parent_index_)
              .ToHandleChecked());
     current_parent_index_ = info.GetParentIndex();
   }
 
   // Saves full information about a function: its code, its scope info
   // and a SharedFunctionInfo object.
   void FunctionInfo(Handle<SharedFunctionInfo> shared, Scope* scope,
                     Zone* zone) {
-    if (!shared->IsSharedFunctionInfo()) {
-      return;
-    }
+    DCHECK(shared->IsSharedFunctionInfo());

[Yang, 2016/05/11 07:29:09]

We should not need this check. I don't even know why we had this in the first
place.

[jgruber1, 2016/05/12 12:30:25]

Done.

     FunctionInfoWrapper info = FunctionInfoWrapper::cast(
         *JSReceiver::GetElement(isolate(), result_, current_parent_index_)
              .ToHandleChecked());
     info.SetFunctionCode(Handle<Code>(shared->code()),
                          Handle<HeapObject>(shared->scope_info()));
     info.SetSharedFunctionInfo(shared);
 
     Handle<Object> scope_info_list = SerializeFunctionScope(scope, zone);
     info.SetFunctionScopeInfo(scope_info_list);
   }
@@ skipping 1250 matching lines @@
   }
   if (target.saved_status() == LiveEdit::FUNCTION_BLOCKED_UNDER_NATIVE_CODE) {
     return "Function is blocked under native code";
   }
   if (target.saved_status() == LiveEdit::FUNCTION_BLOCKED_UNDER_GENERATOR) {
     return "Function is blocked under a generator activation";
   }
   return NULL;
 }
 
+// Traverses the entire AST, and records information about all FunctionLiterals
+// for further use by LiveEdit code patching.
+class LiveEditFunctionVisitor : public AstTraversalVisitor {
+ public:
+  static void Visit(FunctionLiteral* node, Handle<Script> script, Zone* zone,
+                    Isolate* isolate);
 
-LiveEditFunctionTracker::LiveEditFunctionTracker(Isolate* isolate,
-                                                 FunctionLiteral* fun)
-    : isolate_(isolate) {
-  if (isolate_->active_function_info_listener() != NULL) {
-    isolate_->active_function_info_listener()->FunctionStarted(fun);
-  }
+  virtual ~LiveEditFunctionVisitor() {}
+  void VisitFunctionLiteral(FunctionLiteral* node) override;
+
+ private:
+  LiveEditFunctionVisitor(Handle<Script> script, Zone* zone, Isolate* isolate);
+
+  Handle<SharedFunctionInfo> FindSharedFunctionInfo(FunctionLiteral* fun);
+
+  FunctionInfoListener* function_info_listener_;
+  Handle<Script> script_;
+  Zone* zone_;
+
+  DISALLOW_COPY_AND_ASSIGN(LiveEditFunctionVisitor);
+};
+
+void LiveEditFunctionVisitor::Visit(FunctionLiteral* node,
+                                    Handle<Script> script, Zone* zone,
+                                    Isolate* isolate) {
+  LiveEditFunctionVisitor visitor(script, zone, isolate);
+  visitor.VisitFunctionLiteral(node);
 }
 
-
-LiveEditFunctionTracker::~LiveEditFunctionTracker() {
-  if (isolate_->active_function_info_listener() != NULL) {
-    isolate_->active_function_info_listener()->FunctionDone();
-  }
+LiveEditFunctionVisitor::LiveEditFunctionVisitor(Handle<Script> script,
+                                                 Zone* zone, Isolate* isolate)
+    : AstTraversalVisitor(isolate), script_(script), zone_(zone) {
+  function_info_listener_ = isolate->active_function_info_listener();
+  DCHECK_NOT_NULL(function_info_listener_);
 }
 
-
-void LiveEditFunctionTracker::RecordFunctionInfo(
-    Handle<SharedFunctionInfo> info, FunctionLiteral* lit,
-    Zone* zone) {
-  if (isolate_->active_function_info_listener() != NULL) {
-    isolate_->active_function_info_listener()->FunctionInfo(info, lit->scope(),
-                                                            zone);
+Handle<SharedFunctionInfo> LiveEditFunctionVisitor::FindSharedFunctionInfo(
+    FunctionLiteral* fun) {
+  // This is a near-duplicate of Script::FindSharedFunctionInfo, with the
+  // difference being that here we also need to access top-level functions.
+  // Possibly remove the assertion from Script:FindSharedFunctionInfo to
+  // avoid duplicate code.

[Yang, 2016/05/11 07:29:09]

Yup. Let's use Script::FindSharedFunctionInfo instead. The (now wrong) assertion
in it should be moved into its other call site, GetSharedFunctionInfo.

[jgruber1, 2016/05/12 12:30:25]

Done.

+  WeakFixedArray::Iterator iterator(script_->shared_function_infos());
+  SharedFunctionInfo* shared;
+  while ((shared = iterator.Next<SharedFunctionInfo>())) {
+    if (fun->function_token_position() == shared->function_token_position() &&
+        fun->start_position() == shared->start_position() &&
+        fun->end_position() == shared->end_position()) {
+      return Handle<SharedFunctionInfo>(shared);
+    }
   }
+  UNREACHABLE();
+  return Handle<SharedFunctionInfo>();
 }
 
+void LiveEditFunctionVisitor::VisitFunctionLiteral(FunctionLiteral* node) {
+  Scope* scope = node->scope();
+
+  // FunctionStarted is called in pre-order.
+  function_info_listener_->FunctionStarted(node);
+
+  VisitDeclarations(scope->declarations());
+  VisitStatements(node->body());
+
+  // FunctionInfo and FunctionDone are called in post-order.
+  Handle<SharedFunctionInfo> info = FindSharedFunctionInfo(node);

[Yang, 2016/05/11 07:29:09]

For the number of shared function infos in the script, the complexity of
FindSharedFunctionInfo is O(n), so for the whole script we are looking at
O(n^2). I'm not too worried about this right now, but we could add a TODO here
to optimize this if this becomes a problem. For example, for large scripts with
many entries in the list, sort upfront and do a binary search.

[jgruber1, 2016/05/12 12:30:25]

Good point. The repeated linear search is pretty ugly. I've added a TODO for
now.

+  function_info_listener_->FunctionInfo(info, scope, zone_);
+  function_info_listener_->FunctionDone();
+}
+
+void LiveEditFunctionTracker::Collect(Handle<Script> script,
+                                      ParseInfo* parse_info, Isolate* isolate) {
+  LiveEditFunctionVisitor::Visit(parse_info->literal(), script,
+                                 parse_info->zone(), isolate);
+}
 
 bool LiveEditFunctionTracker::IsActive(Isolate* isolate) {
   return isolate->active_function_info_listener() != NULL;
 }
 
 }  // namespace internal
 }  // namespace v8