Remove Enqueuer argument from Backend.registerStaticUse

Remove Enqueuer argument from Backend.registerStaticUse

- and from CustomElementsAnalysis, TypeVariableHandler, and LookupMapAnalysis methods

R=sigmund@google.com

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

[Johnni Winther, 2016-09-29 08:17:07]

Description was changed from

==========
Remove Enqueuer argument from Backend.registerStaticUse
==========

to

==========
Remove Enqueuer argument from Backend.registerStaticUse

- and from CustomElementsAnalysis methods
==========

[Johnni Winther, 2016-09-29 08:17:17]

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

[Johnni Winther, 2016-09-29 08:43:55]

Description was changed from

==========
Remove Enqueuer argument from Backend.registerStaticUse

- and from CustomElementsAnalysis methods
==========

to

==========
Remove Enqueuer argument from Backend.registerStaticUse

- and from CustomElementsAnalysis, TypeVariableHandler, and LookupMapAnalysis
methods
==========

[Siggi Cherem (dart-lang), 2016-09-29 18:04:44]

lgtm, with one small suggestion, and a few general questions about what to do
with impact-strategy (for a separate CL)

https://codereview.chromium.org/2378063002/diff/60001/pkg/compiler/lib/src/en...
File pkg/compiler/lib/src/enqueue.dart (right):

https://codereview.chromium.org/2378063002/diff/60001/pkg/compiler/lib/src/en...
pkg/compiler/lib/src/enqueue.dart:115: /// the impact strategy will remove it
from the element impact cache, if it is
a few questions here...

1. other than reducing memory-use, any other reason we need to clear the cache
one by one?

2. any reason we have the impact-strategy in the backend? it seems that the
strategy there is pretty general and not JS specific.

3. are we gaining much from having the impact-strategy abstraction? It appears
that we mainly use it to decide how we deal with caching, so I'm wondering if
things would get simpler if we deal with caching directly.

https://codereview.chromium.org/2378063002/diff/60001/pkg/compiler/lib/src/en...
pkg/compiler/lib/src/enqueue.dart:117: void applyImpact(WorldImpact worldImpact,
{Element forElement});
nit: rename "forElement" to "impactSource"?

https://codereview.chromium.org/2378063002/diff/60001/pkg/compiler/lib/src/js...
File pkg/compiler/lib/src/js_backend/backend.dart (right):

https://codereview.chromium.org/2378063002/diff/60001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_backend/backend.dart:3219: if (impactUse ==
ResolutionEnqueuer.IMPACT_USE) {
unrelated to this CL, but related comment to the impact-strategy:

consider renaming *.IMPACT_USE: since so many impacts are about uses
"static-use", "type-use", "impact-use" sounds like an impact itself :-)

Some ideas:
- rename impactUse => useCase. 
- move the constant to ImpactUseCase.RESOLUTION

if (useCase == ImpactUseCase.RESOLUTION) { ... }

https://codereview.chromium.org/2378063002/diff/60001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_backend/backend.dart:3221: impact.apply(visitor);
unrelated nit: since you intend to call apply on every branch, consider
refactoring it to just do that upfront. For example:

  visitImpact(...) {
    impact.apply(visitor);
    if (useCase == ImpactUseCase.RESOLUTION && !supportDeferredLoad &&
!supportSerialization && element != null) {
      resolution.uncacheWorldImpact(element);
    }
    if (useCase == ImpactUseCase.DUMP_INFO) {
      dumpInfoTask.unregisterImpact(element);
    }
  }

https://codereview.chromium.org/2378063002/diff/60001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_backend/backend.dart:3225:
resolution.uncacheWorldImpact(element);
related to the question earlier about getting rid of impact-strategy. I was
looking more closely at where we do something special about caching, and I'm
more and more inclined to move the logic directly to those places and remove the
strategy abstraction.

That would mean that the resolution enqueuer would directly clear it's cache, so
instead of:

   compiler.impactStrategy
       .visitImpact(element, worldImpact, impactVisitor, impactUse);

we'd have:

   worldImpact.apply(impactVisitor);
   if (!supportDeferredLoad && !supportSerialization && element != null) {
     resolution.uncacheWorldImpact(element);
   }


this also means that ImpactUseCase might disappear.

[Johnni Winther, 2016-09-30 08:26:11]

Description was changed from

==========
Remove Enqueuer argument from Backend.registerStaticUse

- and from CustomElementsAnalysis, TypeVariableHandler, and LookupMapAnalysis
methods
==========

to

==========
Remove Enqueuer argument from Backend.registerStaticUse

- and from CustomElementsAnalysis, TypeVariableHandler, and LookupMapAnalysis
methods

R=sigmund@google.com

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

[Johnni Winther, 2016-09-30 08:26:13]

Committed patchset #5 (id:80001) manually as
ead7f7030afbb7e1fdc8e6fe1679f89bcc4ff6fe (presubmit successful).

[Johnni Winther, 2016-09-30 08:27:03]

https://codereview.chromium.org/2378063002/diff/60001/pkg/compiler/lib/src/en...
File pkg/compiler/lib/src/enqueue.dart (right):

https://codereview.chromium.org/2378063002/diff/60001/pkg/compiler/lib/src/en...
pkg/compiler/lib/src/enqueue.dart:115: /// the impact strategy will remove it
from the element impact cache, if it is
On 2016/09/29 18:04:43, Siggi Cherem (dart-lang) wrote:
> a few questions here...
> 
> 1. other than reducing memory-use, any other reason we need to clear the cache
> one by one?
> 
> 2. any reason we have the impact-strategy in the backend? it seems that the
> strategy there is pretty general and not JS specific.
> 
> 3. are we gaining much from having the impact-strategy abstraction? It appears
> that we mainly use it to decide how we deal with caching, so I'm wondering if
> things would get simpler if we deal with caching directly.

1. For memory-use.
2. It was (or is) planned to skip application of previously applied
BackendImpacts when we don't need deferred or dart2js-info, but has not been
implemented (yet).
3. Since we no longer need to support multiple targets we could inline the
reasoning.

https://codereview.chromium.org/2378063002/diff/60001/pkg/compiler/lib/src/en...
pkg/compiler/lib/src/enqueue.dart:117: void applyImpact(WorldImpact worldImpact,
{Element forElement});
On 2016/09/29 18:04:43, Siggi Cherem (dart-lang) wrote:
> nit: rename "forElement" to "impactSource"?

Done.

https://codereview.chromium.org/2378063002/diff/60001/pkg/compiler/lib/src/js...
File pkg/compiler/lib/src/js_backend/backend.dart (right):

https://codereview.chromium.org/2378063002/diff/60001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_backend/backend.dart:3219: if (impactUse ==
ResolutionEnqueuer.IMPACT_USE) {
On 2016/09/29 18:04:43, Siggi Cherem (dart-lang) wrote:
> unrelated to this CL, but related comment to the impact-strategy:
> 
> consider renaming *.IMPACT_USE: since so many impacts are about uses
> "static-use", "type-use", "impact-use" sounds like an impact itself :-)
> 
> Some ideas:
> - rename impactUse => useCase. 
> - move the constant to ImpactUseCase.RESOLUTION
> 
> if (useCase == ImpactUseCase.RESOLUTION) { ... }

Acknowledged.

https://codereview.chromium.org/2378063002/diff/60001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_backend/backend.dart:3221: impact.apply(visitor);
On 2016/09/29 18:04:43, Siggi Cherem (dart-lang) wrote:
> unrelated nit: since you intend to call apply on every branch, consider
> refactoring it to just do that upfront. For example:
> 
>   visitImpact(...) {
>     impact.apply(visitor);
>     if (useCase == ImpactUseCase.RESOLUTION && !supportDeferredLoad &&
> !supportSerialization && element != null) {
>       resolution.uncacheWorldImpact(element);
>     }
>     if (useCase == ImpactUseCase.DUMP_INFO) {
>       dumpInfoTask.unregisterImpact(element);
>     }
>   }
> 

Acknowledged.

https://codereview.chromium.org/2378063002/diff/60001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_backend/backend.dart:3225:
resolution.uncacheWorldImpact(element);
On 2016/09/29 18:04:43, Siggi Cherem (dart-lang) wrote:
> related to the question earlier about getting rid of impact-strategy. I was
> looking more closely at where we do something special about caching, and I'm
> more and more inclined to move the logic directly to those places and remove
the
> strategy abstraction.
> 
> That would mean that the resolution enqueuer would directly clear it's cache,
so
> instead of:
> 
>    compiler.impactStrategy
>        .visitImpact(element, worldImpact, impactVisitor, impactUse);
> 
> we'd have:
> 
>    worldImpact.apply(impactVisitor);
>    if (!supportDeferredLoad && !supportSerialization && element != null) {
>      resolution.uncacheWorldImpact(element);
>    }
> 
> 
> this also means that ImpactUseCase might disappear.

Acknowledged.