Track emitted classes in incremental compiler.

Track emitted classes in incremental compiler.

R=johnniwinther@google.com

Committed: https://code.google.com/p/dart/source/detail?r=42282

[ahe, 2014-12-01 08:31:02]

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

[ahe, 2014-12-01 08:31:02]

Florian: FYI, I've made changes to code_emitter_task.dart.

[karlklose, 2014-12-01 10:13:31]

karlklose@google.com changed reviewers:
+ karlklose@google.com

[karlklose, 2014-12-01 10:13:32]

DBC:

https://codereview.chromium.org/768993003/diff/20001/dart/pkg/compiler/lib/sr...
File dart/pkg/compiler/lib/src/js_emitter/code_emitter_task.dart (right):

https://codereview.chromium.org/768993003/diff/20001/dart/pkg/compiler/lib/sr...
dart/pkg/compiler/lib/src/js_emitter/code_emitter_task.dart:326: void
computeNeeds() {
Perhaps you can find a better name than "needs". How about
"computeEntitiesToEmit"?

At least the, function should have a comment that describes what it is supposed
to compute.

https://codereview.chromium.org/768993003/diff/20001/dart/pkg/dart2js_increme...
File dart/pkg/dart2js_incremental/lib/library_updater.dart (right):

https://codereview.chromium.org/768993003/diff/20001/dart/pkg/dart2js_increme...
dart/pkg/dart2js_incremental/lib/library_updater.dart:603: // Ask the emitter to
compute "needs" (only) if new classes were
Please document what "needs" are.

https://codereview.chromium.org/768993003/diff/20001/dart/pkg/dart2js_increme...
dart/pkg/dart2js_incremental/lib/library_updater.dart:612: emitter.neededClasses
Why not `new Set.fom`?

[ahe, 2014-12-01 10:28:36]

Thanks for taking a look, Karl. Could you please take a look at my questions
below?

https://codereview.chromium.org/768993003/diff/20001/dart/pkg/compiler/lib/sr...
File dart/pkg/compiler/lib/src/js_emitter/code_emitter_task.dart (right):

https://codereview.chromium.org/768993003/diff/20001/dart/pkg/compiler/lib/sr...
dart/pkg/compiler/lib/src/js_emitter/code_emitter_task.dart:326: void
computeNeeds() {
On 2014/12/01 10:13:31, karlklose wrote:
> Perhaps you can find a better name than "needs". How about
> "computeEntitiesToEmit"?

Since most of the methods are called "computedNeededFoo", I think
computeNeed...something is a good fit.

How about: computeNeededEntities?

> At least the, function should have a comment that describes what it is
supposed
> to compute.

I'd rather not add documentation as I'm not 100% sure exactly what is computed.
Or I can add something vague like:

/// Initializes various collections that inform the emitter about what is
needed.

The reason why I'd rather not add documentation is that I'm not 100% sure that
everything that I think should be computed by this method is actually computed
by this method.

WDYT?

https://codereview.chromium.org/768993003/diff/20001/dart/pkg/dart2js_increme...
File dart/pkg/dart2js_incremental/lib/library_updater.dart (right):

https://codereview.chromium.org/768993003/diff/20001/dart/pkg/dart2js_increme...
dart/pkg/dart2js_incremental/lib/library_updater.dart:603: // Ask the emitter to
compute "needs" (only) if new classes were
On 2014/12/01 10:13:32, karlklose wrote:
> Please document what "needs" are.

I'm being vague because I don't know precisely.

https://codereview.chromium.org/768993003/diff/20001/dart/pkg/dart2js_increme...
dart/pkg/dart2js_incremental/lib/library_updater.dart:612: emitter.neededClasses
On 2014/12/01 10:13:31, karlklose wrote:
> Why not `new Set.fom`?

The field is final.

[floitsch, 2014-12-01 13:12:28]

floitsch@google.com changed reviewers:
+ floitsch@google.com

[floitsch, 2014-12-01 13:12:29]

dbc.

https://codereview.chromium.org/768993003/diff/20001/dart/pkg/dart2js_increme...
File dart/pkg/dart2js_incremental/lib/library_updater.dart (right):

https://codereview.chromium.org/768993003/diff/20001/dart/pkg/dart2js_increme...
dart/pkg/dart2js_incremental/lib/library_updater.dart:573: for (Selector
selector in compiler.enqueuer.codegen.newlySeenSelectors) {
long line.

[ahe, 2014-12-01 13:58:52]

Thank you, Florian and Karl!

https://codereview.chromium.org/768993003/diff/20001/dart/pkg/compiler/lib/sr...
File dart/pkg/compiler/lib/src/js_emitter/code_emitter_task.dart (right):

https://codereview.chromium.org/768993003/diff/20001/dart/pkg/compiler/lib/sr...
dart/pkg/compiler/lib/src/js_emitter/code_emitter_task.dart:326: void
computeNeeds() {
On 2014/12/01 10:28:35, ahe wrote:
> On 2014/12/01 10:13:31, karlklose wrote:
> > Perhaps you can find a better name than "needs". How about
> > "computeEntitiesToEmit"?
> 
> Since most of the methods are called "computedNeededFoo", I think
> computeNeed...something is a good fit.
> 
> How about: computeNeededEntities?
> 
> > At least the, function should have a comment that describes what it is
> supposed
> > to compute.
> 
> I'd rather not add documentation as I'm not 100% sure exactly what is
computed.
> Or I can add something vague like:
> 
> /// Initializes various collections that inform the emitter about what is
> needed.
> 
> The reason why I'd rather not add documentation is that I'm not 100% sure that
> everything that I think should be computed by this method is actually computed
> by this method.
> 
> WDYT?

Discussed offline: I've renamed the method to computeAllNeededEntities.

https://codereview.chromium.org/768993003/diff/20001/dart/pkg/dart2js_increme...
File dart/pkg/dart2js_incremental/lib/library_updater.dart (right):

https://codereview.chromium.org/768993003/diff/20001/dart/pkg/dart2js_increme...
dart/pkg/dart2js_incremental/lib/library_updater.dart:573: for (Selector
selector in compiler.enqueuer.codegen.newlySeenSelectors) {
On 2014/12/01 13:12:29, floitsch wrote:
> long line.

Done.

[Johnni Winther, 2014-12-09 12:01:45]

lgtm

[ahe, 2014-12-11 09:40:15]

Please review my conflict resolution. I had three conflicts, and I've marked
them with a comment.

https://codereview.chromium.org/768993003/diff/120001/dart/pkg/compiler/lib/s...
File dart/pkg/compiler/lib/src/js_emitter/code_emitter_task.dart (right):

https://codereview.chromium.org/768993003/diff/120001/dart/pkg/compiler/lib/s...
dart/pkg/compiler/lib/src/js_emitter/code_emitter_task.dart:355:
typeTestRegistry.computeRequiredTypeChecks();
Conflict resolution: typeTestRegistry was renamed.

https://codereview.chromium.org/768993003/diff/120001/dart/pkg/dart2js_increm...
File dart/pkg/dart2js_incremental/lib/library_updater.dart (right):

https://codereview.chromium.org/768993003/diff/120001/dart/pkg/dart2js_increm...
dart/pkg/dart2js_incremental/lib/library_updater.dart:145: ? [] :
compiler.codegenWorld.directlyInstantiatedClasses);
Conflict resolution.

https://codereview.chromium.org/768993003/diff/120001/dart/pkg/dart2js_increm...
dart/pkg/dart2js_incremental/lib/library_updater.dart:580: for (Selector
selector in enqueuer.codegen.newlySeenSelectors) {
Conflict resolution here (no change needed as long line had been addressed
differently in previous CL).

[Johnni Winther, 2014-12-11 13:04:44]

lgtm

[ahe, 2014-12-11 13:12:56]

Committed patchset #7 (id:120001) manually as 42282 (presubmit successful).