Add ServerOperation and queue.

pkg/analysis_server/lib/src/analysis_server.dart

Index: pkg/analysis_server/lib/src/analysis_server.dart
diff --git a/pkg/analysis_server/lib/src/analysis_server.dart b/pkg/analysis_server/lib/src/analysis_server.dart
index ef7bfcb458cc3c9a7f9c8aec542791f906d3bdc3..377915328ec4d64e3f7bdc402c771bad23776b06 100644
--- a/pkg/analysis_server/lib/src/analysis_server.dart
+++ b/pkg/analysis_server/lib/src/analysis_server.dart
@@ -11,6 +11,8 @@ import 'package:analysis_server/src/channel.dart';
 import 'package:analysis_server/src/constants.dart';
 import 'package:analysis_server/src/context_directory_manager.dart';
 import 'package:analysis_server/src/domain_analysis.dart';
+import 'package:analysis_server/src/operation/operation.dart';
+import 'package:analysis_server/src/operation/operation_queue.dart';
 import 'package:analysis_server/src/protocol.dart';
 import 'package:analysis_server/src/resource.dart';
 import 'package:analyzer/src/generated/ast.dart';
@@ -40,7 +42,7 @@ class AnalysisServerContextDirectoryManager extends ContextDirectoryManager {
     ContextDirectory contextDirectory = new ContextDirectory(
         analysisServer.defaultSdk, folder, pubspecFile);
     analysisServer.folderMap[folder] = contextDirectory;
-    analysisServer.addContextToWorkQueue(contextDirectory.context);
+    analysisServer.schedulePerformAnalysisOperation(contextDirectory.context, false);
   }
 
   void applyChangesToContext(Folder contextFolder, ChangeSet changeSet) {
@@ -78,17 +80,6 @@ class AnalysisServer {
    */
   List<RequestHandler> handlers;
 
-  // TODO(scheglov) remove once setAnalysisRoots() is completely implemented
-//  /**
-//   * A table mapping context id's to the analysis contexts associated with them.
-//   */
-//  final Map<String, AnalysisContext> contextMap = new Map<String, AnalysisContext>();
-//
-//  /**
-//   * A table mapping analysis contexts to the context id's associated with them.
-//   */
-//  final Map<AnalysisContext, String> contextIdMap = new Map<AnalysisContext, String>();
-
   /**
    * The current default [DartSdk].
    */
@@ -104,15 +95,7 @@ class AnalysisServer {
    */
   String lastStatusNotificationContextId = null;
 
-  /**
-   * A list of the analysis contexts for which analysis work needs to be
-   * performed.
-   *
-   * Invariant: when this list is non-empty, there is exactly one pending call

[Paul Berry, 2014/06/02 16:04:41]

Why is this comment being deleted?  Is your intent to establish a different
invariant?  If so, what?  I think this invariant was a good one.

[scheglov, 2014/06/02 17:56:59]

Restored for the operation queue field.

-   * to [performTask] on the event queue.  When this list is empty, there are
-   * no calls to [performTask] on the event queue.
-   */
-  final List<AnalysisContext> contextWorkQueue = new List<AnalysisContext>();
+  final ServerOperationQueue operationQueue = new ServerOperationQueue();
 
   /**
    * A set of the [ServerService]s to send notifications for.
@@ -139,21 +122,17 @@ class AnalysisServer {
   }
 
   /**
-   * If [running] is true, add the given [context] to the list of analysis
-   * contexts for which analysis work needs to be performed, and ensure that
-   * the work will be performed.
+   * Schedules analysis of the given context.
+   *
+   * [isContinue] is `true` if the new operation is continuation of analysis of
+   * the same context which was analyzed before.
    */
-  void addContextToWorkQueue(AnalysisContext context) {
-    if (!running) {
-      return;
-    }
-    if (!contextWorkQueue.contains(context)) {
-      contextWorkQueue.add(context);
-      if (contextWorkQueue.length == 1) {
-        // Work queue was previously empty, so schedule analysis.
-        _scheduleTask();
-      }
-    }
+  void schedulePerformAnalysisOperation(AnalysisContext context, bool isContinue) {
+//    bool isPriority = priorityContexts.contains(contextId);
+    // TODO(scheglov) support for priority sources
+    bool isPriority = false;
+    operationQueue.add(new PerformAnalysisOperation(context, isPriority, isContinue));
+    _scheduleTask();

[Brian Wilkerson, 2014/06/02 14:32:18]

I believe that this will cause the event queue to become flooded with calls to
performTask (because performTask also schedules itself).

I think the better design is to have a single method that adds an operation to
the queue and conditionally schedules performTask to be run if the queue was
previously empty. Perhaps we should pass a closure (_scheduleTask) into "add"
that gets run iff the queue was previously empty.

[Paul Berry, 2014/06/02 16:04:41]

I'm not sure we even need a closure.  I think it is sufficient to do:

bool wasEmpty = operationQueue.isEmpty;
operationQueue.add(...);
if (wasEmpty) {
  _scheduleTask();
}

(which is essentially the logic we had before this CL, except that previously we
had a .length method so we could simply check whether length was 1 after adding
the task).

   }
 
   /**
@@ -192,54 +171,47 @@ class AnalysisServer {
   }
 
   /**
-   * Perform the next available task. If a request was received that has not yet
-   * been performed, perform it next. Otherwise, look for some analysis that
-   * needs to be done and do that. Otherwise, do nothing.
+   * Perform the next available [ServerOperation].
    */
   void performTask() {
     if (!running) {
-      // An error has occurred, or the connection to the client has been
-      // closed, since performTask() was scheduled on the event queue.  So
-      // don't do any analysis.  Instead clear the work queue.
-      contextWorkQueue.clear();

[Paul Berry, 2014/06/02 16:04:41]

Why are we getting rid of this code?  Was it incorrect?

[scheglov, 2014/06/02 17:56:59]

Restored.

+      return;
     }
-    if (contextWorkQueue.isEmpty) {
-      // Nothing to do.
+    // prepare next operation
+    ServerOperation operation = operationQueue.take();
+    if (operation == null) {
       return;
     }
-    //
-    // Look for a context that has work to be done and then perform one task.
-    //
-    List<ChangeNotice> notices = null;
-//    String contextId;
+    // perform the operation
     try {
-      AnalysisContext context = contextWorkQueue[0];
-//      contextId = contextIdMap[context];
-      // TODO(danrubel): Replace with context identifier or similar
-      sendStatusNotification(context.toString());
-      AnalysisResult result = context.performAnalysisTask();
-      notices = result.changeNotices;
-    } finally {

[Paul Berry, 2014/06/02 16:04:41]

Why are we dropping this logic?  I believe that scheduling the next task in a
finally block is the right thing to do.  If we don't, then it means that a
single exception will effectively grind the whole server to a halt, because it
breaks the invariant.

[scheglov, 2014/06/02 17:56:59]

Restored in performTask/performOperation.

-      if (notices == null) {
-        // Either we have no more work to do for this context, or there was an
-        // unhandled exception trying to perform the analysis.  In either case,
-        // remove the context form the work queue so we won't try to do more
-        // analysis on it.
-        contextWorkQueue.removeAt(0);
-      }
-      //
-      // Schedule this method to be run again if there is any more work to be
-      // done.
-      //
-      if (!contextWorkQueue.isEmpty) {
-        _scheduleTask();
-      }
+      operation.perform(this);
+    } catch (e) {
+      // TODO(scheglov) decide how to handle exceptions
+    }
+    // schedule this method again
+    _scheduleTask();

[Brian Wilkerson, 2014/06/02 14:32:18]

This method should only get scheduled if the queue is not empty.

[scheglov, 2014/06/02 17:56:59]

Done.

+  }
+
+  /**
+   * Perform analysis in the given [AnalysisContext].
+   */
+  void internalPerformAnalysis(AnalysisContext context) {

[Brian Wilkerson, 2014/06/02 14:32:18]

I would prefer a style in which the code to perform an operation were in the
operation's perform() method rather than having to implement an internal method
for every operation.

[scheglov, 2014/06/02 17:56:59]

Me too, but I'm afraid we have to expose too much internals of AnalysisServer to
do so.

[Brian Wilkerson, 2014/06/02 18:21:03]

I'd like to start with doing it the better way until we know that it won't work.

+    // prepare results
+    AnalysisResult result = context.performAnalysisTask();
+    List<ChangeNotice> notices = result.changeNotices;
+    if (notices == null) {
+      return;
     }
+    // TODO(scheglov) remember known sources
+    // TODO(scheglov) index units
+    // TODO(scheglov) schedule notifications
     if (notices != null) {
       sendNotices(notices);
     } else {
       sendStatusNotification(null);
     }
+    // schedule analysis again
+    schedulePerformAnalysisOperation(context, true);

[Brian Wilkerson, 2014/06/02 14:32:18]

We need to take the "continuation" flag into account when scheduling analysis
operations. Otherwise we're going to be using more memory than necessary at any
given time and for longer periods of time.

[scheglov, 2014/06/02 17:56:59]

We do use it - PerformAnalysisOperation returns different priority depending on
if it is a continuation or not.

[Brian Wilkerson, 2014/06/02 18:21:03]

I meant, if we are continuing analysis of a context then that operation should
be added at the head of the queue rather than the tail.

[scheglov, 2014/06/02 18:32:25]

Well, it is added to the tail of the ANALYSIS_CONTINUE queue, which is
practically always has 0 or 1 item.

   }
 
   /**
@@ -341,7 +313,7 @@ class AnalysisServer {
           analysisContext.setChangedContents(source, change.content,
               change.offset, change.oldLength, change.newLength);
         }
-        addContextToWorkQueue(analysisContext);
+        schedulePerformAnalysisOperation(analysisContext, false);
       }
     });
   }
@@ -418,7 +390,7 @@ class AnalysisServer {
    * Return `true` if all tasks are finished in this [AnalysisServer].
    */
   bool test_areTasksFinished() {
-    return contextWorkQueue.isEmpty;
+    return operationQueue.isEmpty;
   }
 
   static Map<String, Object> errorToJson(AnalysisError analysisError) {