Add WorkItemBuilder to abstract WorkItem creation from the enqueuers.

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

 // Copyright (c) 2012, the Dart project authors.  Please see the AUTHORS file
 // for details. All rights reserved. Use of this source code is governed by a
 // BSD-style license that can be found in the LICENSE file.
 
 library dart2js.js.enqueue;
 
 import 'dart:collection' show Queue;
 
 import '../cache_strategy.dart' show CacheStrategy;
 import '../common/backend_api.dart' show Backend;
 import '../common/codegen.dart' show CodegenWorkItem;
 import '../common/tasks.dart' show CompilerTask;
 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;
 import '../options.dart';
 import '../types/types.dart' show TypeMaskStrategy;
 import '../universe/world_builder.dart';
 import '../universe/use.dart'
     show DynamicUse, StaticUse, StaticUseKind, TypeUse, TypeUseKind;
 import '../universe/world_impact.dart'
     show ImpactUseCase, WorldImpact, WorldImpactVisitor;
 import '../util/enumset.dart';
 import '../util/util.dart' show Setlet;
 
 /// [Enqueuer] which is specific to code generation.
 class CodegenEnqueuer extends EnqueuerImpl {
   final String name;
   final EnqueuerStrategy strategy;
 
   Set<ClassEntity> _recentClasses = new Setlet<ClassEntity>();
   final CodegenWorldBuilderImpl _universe;
+  final WorkItemBuilder _workItemBuilder;
 
   bool queueIsClosed = false;
   final CompilerTask task;
   final native.NativeEnqueuer nativeEnqueuer;
   final Backend _backend;
   final CompilerOptions _options;
 
   WorldImpactVisitor _impactVisitor;
 
   final Queue<WorkItem> _queue = new Queue<WorkItem>();
 
   /// All declaration elements that have been processed by codegen.
   final Set<Entity> _processedEntities = new Set<Entity>();
 
   final Set<Entity> newlyEnqueuedElements;
 
   final Set<DynamicUse> newlySeenSelectors;
 
   static const ImpactUseCase IMPACT_USE =
       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);
   }
 
   CodegenWorldBuilder get universe => _universe;
 
   bool get queueIsEmpty => _queue.isEmpty;
 
   /// 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}) {
     if (worldImpact.isEmpty) return;
     impactStrategy.visitImpact(
         impactSource, worldImpact, _impactVisitor, impactUse);
   }
 
   void _registerInstantiatedType(InterfaceType type,
       {bool mirrorUsage: false, bool nativeUsage: false}) {
@@ skipping 157 matching lines @@
     _universe.forgetElement(entity, compiler);
     _processedEntities.remove(entity);
   }
 
   @override
   Iterable<Entity> get processedEntities => _processedEntities;
 
   @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);
+  }
+}