Add WorkItemBuilder to abstract WorkItem creation from the enqueuers.

pkg/compiler/lib/src/js_backend/enqueuer.dart

Index: pkg/compiler/lib/src/js_backend/enqueuer.dart
diff --git a/pkg/compiler/lib/src/js_backend/enqueuer.dart b/pkg/compiler/lib/src/js_backend/enqueuer.dart
index ae34a58f850a8cb3104359b86ed24136534c7318..55fda27f976dbf85c98765970d3472b1c8c3227d 100644
--- a/pkg/compiler/lib/src/js_backend/enqueuer.dart
+++ b/pkg/compiler/lib/src/js_backend/enqueuer.dart
@@ -14,7 +14,7 @@ import '../common/work.dart' show WorkItem;
 import '../common.dart';
 import '../compiler.dart' show Compiler;
 import '../dart_types.dart' show DartType, InterfaceType;
-import '../elements/elements.dart' show Element, Entity, TypedElement;
+import '../elements/elements.dart' show Entity, MemberElement, TypedElement;
 import '../elements/entities.dart';
 import '../enqueue.dart';
 import '../native/native.dart' as native;
@@ -35,6 +35,7 @@ class CodegenEnqueuer extends EnqueuerImpl {
 
   Set<ClassEntity> _recentClasses = new Setlet<ClassEntity>();
   final CodegenWorldBuilderImpl _universe;
+  final WorkItemBuilder _workItemBuilder;
 
   bool queueIsClosed = false;
   final CompilerTask task;
@@ -57,13 +58,15 @@ class CodegenEnqueuer extends EnqueuerImpl {
       const ImpactUseCase('CodegenEnqueuer');
 
   CodegenEnqueuer(this.task, CacheStrategy cacheStrategy, Backend backend,
-      this._options, this.strategy)
+      CompilerOptions options, this.strategy)
       : _universe =
             new CodegenWorldBuilderImpl(backend, const TypeMaskStrategy()),
+        _workItemBuilder = new CodegenWorkItemBuilder(backend, options),

[Siggi Cherem (dart-lang), 2016/12/20 21:21:47]

You might be thinking of this already, but can we take this a step further and
get rid of WorkItem altogether?

It seems to me that:
 - WorkItem.run() is not called anywhere (except in unittests with useMock,
which we can address in a special way anyways). 
 - WorkItems are just a wrapper on an entity

So, we could make the queue directly hold entities instead.

You still need something to decide whether to add an entity to the queue, so the
logic here would become an entity filter, rather than a workitem builder.

Thoughts?

If you like this idea, but removing workItem is a lot of extra work for this CL,
how about we use the idea of filters but continue using WorkItem a while longer?
That would make _addToWorkList more like:

  if (_processedEntities.contains(entity)) return;
  if (_skipEntity(entity)) return;
  ...
  _queue.add(new CodegenWorkItem(_backend, entity));

[Johnni Winther, 2016/12/21 10:19:37]

This move is needed to, eventually, create kernel based resolution work-items
and kernel-to-ssa based codegen work item. The enqueuer shouldn't case what
constitutes a work-item.

Also the createWorkItem signature might in time be changed to

  Iterable<WorkItem> createWorkItem(MemberEntity entity, EnumSet<MemberUse>
useSet);

making the filtering stand less out.

[Siggi Cherem (dart-lang), 2016/12/21 15:08:53]

I'm not sure I fully understand, we can chat more on our sync up, but I was
thinking we'd enqueue a MemberEntity and nothing else => the kernel based work
items would be just kernel nodes like procedures?

If the enqueuer doesn't care about what's in it, maybe we should make it generic
and store directly what we need on each case? Basically, I'm not convinced we
need the extra WorkItem wrapper class in the end.

         newlyEnqueuedElements = cacheStrategy.newSet(),
         newlySeenSelectors = cacheStrategy.newSet(),
         nativeEnqueuer = backend.nativeCodegenEnqueuer(),
         this._backend = backend,
+        this._options = options,
         this.name = 'codegen enqueuer' {
     _impactVisitor = new EnqueuerImplImpactVisitor(this);
   }
@@ -75,37 +78,25 @@ class CodegenEnqueuer extends EnqueuerImpl {
   /// Returns [:true:] if this enqueuer is the resolution enqueuer.
   bool get isResolutionQueue => false;
 
-  /**
-   * Documentation wanted -- johnniwinther
-   *
-   * Invariant: [element] must be a declaration element.
-   */
-  void _addToWorkList(Element element) {
-    assert(invariant(element, element.isDeclaration));
-    // Don't generate code for foreign elements.
-    if (_backend.isForeign(element)) return;
-    if (element.isAbstract) return;
+  /// Create a [WorkItem] for [entity] and add it to the work list if it has not
+  /// already been processed.
+  void _addToWorkList(MemberEntity entity) {
+    if (_processedEntities.contains(entity)) return;
 
-    // Codegen inlines field initializers. It only needs to generate
-    // code for checked setters.
-    if (element.isField && element.isInstanceMember) {
-      if (!_options.enableTypeAssertions ||
-          element.enclosingElement.isClosure) {
-        return;
-      }
-    }
+    WorkItem workItem = _workItemBuilder.createWorkItem(entity);
+    if (workItem == null) return;
 
-    if (_options.hasIncrementalSupport &&
-        !_processedEntities.contains(element)) {
-      newlyEnqueuedElements.add(element);
+    if (_options.hasIncrementalSupport) {
+      newlyEnqueuedElements.add(entity);
     }
 
     if (queueIsClosed) {
       throw new SpannableAssertionFailure(
-          element, "Codegen work list is closed. Trying to add $element");
+          entity, "Codegen work list is closed. Trying to add $entity");
     }
-    _queue.add(new CodegenWorkItem(_backend, element));
-    applyImpact(_backend.registerUsedElement(element, forResolution: false));
+
+    applyImpact(_backend.registerUsedElement(entity, forResolution: false));
+    _queue.add(workItem);
   }
 
   void applyImpact(WorldImpact worldImpact, {var impactSource}) {
@@ -283,3 +274,30 @@ class CodegenEnqueuer extends EnqueuerImpl {
   @override
   Iterable<ClassEntity> get processedClasses => _universe.processedClasses;
 }
+
+/// Builder that creates the work item necessary for the code generation of a
+/// [MemberElement].
+class CodegenWorkItemBuilder extends WorkItemBuilder {
+  Backend _backend;
+  CompilerOptions _options;
+
+  CodegenWorkItemBuilder(this._backend, this._options);
+
+  @override
+  WorkItem createWorkItem(MemberElement element) {
+    assert(invariant(element, element.isDeclaration));
+    // Don't generate code for foreign elements.
+    if (_backend.isForeign(element)) return null;
+    if (element.isAbstract) return null;
+
+    // Codegen inlines field initializers. It only needs to generate
+    // code for checked setters.
+    if (element.isField && element.isInstanceMember) {
+      if (!_options.enableTypeAssertions ||
+          element.enclosingElement.isClosure) {
+        return null;
+      }
+    }
+    return new CodegenWorkItem(_backend, element);
+  }
+}