Add WorkItemBuilder to abstract WorkItem creation from the enqueuers.

Add WorkItemBuilder to abstract WorkItem creation from the enqueuers.

R=sigmund@google.com

Committed: https://github.com/dart-lang/sdk/commit/2b3c0f18fe35aa3349c5dac3ccc8a93e3b989ff7

[Johnni Winther, 2016-12-20 15:08:28]

johnniwinther@google.com changed reviewers:
+ sigmund@google.com

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

lgtm if you need to get unblocked, but I'd prefer to (a) keep the type-defs
changes separate if they are unrelated, (b) use filters instead of builders, and
eventually (c) get rid of WorkItem :)

https://codereview.chromium.org/2590913002/diff/1/pkg/compiler/lib/src/compil...
File pkg/compiler/lib/src/compiler.dart (right):

https://codereview.chromium.org/2590913002/diff/1/pkg/compiler/lib/src/compil...
pkg/compiler/lib/src/compiler.dart:772: } else if (element.isTypedef) {
I feel I missed something: why typedefs are now handled differently? I didn't
find the relation between that and adding the workitem builder abstration

https://codereview.chromium.org/2590913002/diff/1/pkg/compiler/lib/src/enqueu...
File pkg/compiler/lib/src/enqueue.dart (right):

https://codereview.chromium.org/2590913002/diff/1/pkg/compiler/lib/src/enqueu...
pkg/compiler/lib/src/enqueue.dart:532: WorkItem createWorkItem(MemberEntity
entity);
eventually, should we turn this into just a typedef?

https://codereview.chromium.org/2590913002/diff/1/pkg/compiler/lib/src/js_bac...
File pkg/compiler/lib/src/js_backend/enqueuer.dart (right):

https://codereview.chromium.org/2590913002/diff/1/pkg/compiler/lib/src/js_bac...
pkg/compiler/lib/src/js_backend/enqueuer.dart:64: _workItemBuilder = new
CodegenWorkItemBuilder(backend, options),
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]

https://codereview.chromium.org/2590913002/diff/1/pkg/compiler/lib/src/compil...
File pkg/compiler/lib/src/compiler.dart (right):

https://codereview.chromium.org/2590913002/diff/1/pkg/compiler/lib/src/compil...
pkg/compiler/lib/src/compiler.dart:772: } else if (element.isTypedef) {
On 2016/12/20 21:21:47, Siggi Cherem (dart-lang) wrote:
> I feel I missed something: why typedefs are now handled differently? I didn't
> find the relation between that and adding the workitem builder abstration

Typedef should never have been added to the work-list. The work-item created for
them would effectively call `ensureResolved` as below. They were the last reason
why we couldn't state that all enqueued entities were MemberEntity; which is why
the change is in this CL.

https://codereview.chromium.org/2590913002/diff/1/pkg/compiler/lib/src/enqueu...
File pkg/compiler/lib/src/enqueue.dart (right):

https://codereview.chromium.org/2590913002/diff/1/pkg/compiler/lib/src/enqueu...
pkg/compiler/lib/src/enqueue.dart:532: WorkItem createWorkItem(MemberEntity
entity);
On 2016/12/20 21:21:47, Siggi Cherem (dart-lang) wrote:
> eventually, should we turn this into just a typedef?

We could but it would make the descriptor of the builder function ugly (turning
ResolutionWorkItemBuilder and CodegenWorkItemBuilder item closures)

https://codereview.chromium.org/2590913002/diff/1/pkg/compiler/lib/src/js_bac...
File pkg/compiler/lib/src/js_backend/enqueuer.dart (right):

https://codereview.chromium.org/2590913002/diff/1/pkg/compiler/lib/src/js_bac...
pkg/compiler/lib/src/js_backend/enqueuer.dart:64: _workItemBuilder = new
CodegenWorkItemBuilder(backend, options),
On 2016/12/20 21:21:47, Siggi Cherem (dart-lang) wrote:
> 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));

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.

[Johnni Winther, 2016-12-21 10:37:43]

Description was changed from

==========
Add WorkItemBuilder to abstract WorkItem creation from the enqueuers.
==========

to

==========
Add WorkItemBuilder to abstract WorkItem creation from the enqueuers.

R=sigmund@google.com

Committed:
https://github.com/dart-lang/sdk/commit/2b3c0f18fe35aa3349c5dac3ccc8a93e3b989ff7
==========

[Johnni Winther, 2016-12-21 10:37:44]

Committed patchset #1 (id:1) manually as
2b3c0f18fe35aa3349c5dac3ccc8a93e3b989ff7 (presubmit successful).

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

https://codereview.chromium.org/2590913002/diff/1/pkg/compiler/lib/src/js_bac...
File pkg/compiler/lib/src/js_backend/enqueuer.dart (right):

https://codereview.chromium.org/2590913002/diff/1/pkg/compiler/lib/src/js_bac...
pkg/compiler/lib/src/js_backend/enqueuer.dart:64: _workItemBuilder = new
CodegenWorkItemBuilder(backend, options),
On 2016/12/21 10:19:37, Johnni Winther wrote:
> On 2016/12/20 21:21:47, Siggi Cherem (dart-lang) wrote:
> > 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));
> 
> 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.

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.