[debugger] Refactor LiveEdit function info collection

src/debug/liveedit.cc

Index: src/debug/liveedit.cc
diff --git a/src/debug/liveedit.cc b/src/debug/liveedit.cc
index 2a6eb517b8718dc6396eea282cfbaea0c26f8dde..7b2d71bbb0643620c3ecff02f6a5f39e866d9262 100644
--- a/src/debug/liveedit.cc
+++ b/src/debug/liveedit.cc
@@ -720,9 +720,7 @@ class FunctionInfoListener {
   // 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());
@@ -1993,32 +1991,81 @@ const char* LiveEdit::RestartFrame(JavaScriptFrame* frame) {
   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);
+
+  virtual ~LiveEditFunctionVisitor() {}
+  void VisitFunctionLiteral(FunctionLiteral* node) override;
 
-LiveEditFunctionTracker::LiveEditFunctionTracker(Isolate* isolate,
-                                                 FunctionLiteral* fun)
-    : isolate_(isolate) {
-  if (isolate_->active_function_info_listener() != NULL) {
-    isolate_->active_function_info_listener()->FunctionStarted(fun);
-  }
+ 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);
 }
 
+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_);
+}
 
-LiveEditFunctionTracker::~LiveEditFunctionTracker() {
-  if (isolate_->active_function_info_listener() != NULL) {
-    isolate_->active_function_info_listener()->FunctionDone();
+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();
 
-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);
-  }
+  // 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;