New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

inferred types not treated the same as explicit type by deferred loading #35311

Open
sigmundch opened this Issue Dec 3, 2018 · 2 comments

Comments

Projects
None yet
1 participant
@sigmundch
Member

sigmundch commented Dec 3, 2018

According to the spec it is an error to use a deferred type in an expression or type: you can't use them in variable or function declarations, is/as/catch clauses, and cannot use them in generic type arguments of any form.

This is checked and reported as an error for explicit types in dartanalyzer and CFE, but it is today ignored for inferred types. Unfortunately there is code out there where this occurs. For example, some applications show cases like these:

import 'b.dart' deferred as d;
main() {
   ...
   // inferred return-type in closures:
   d.B f1() => new d.B(); // detected as an error today
   var f2 = () => new d.B(); // no error, but f1 has inferred type: () -> d.B

   // inferred type-arguments
   d.list = <d.B>[]; // detected as an error today, can't use the type parameter
   d.list = []; // no error, but type parameter was injected here
   d.list = d.eList.map((x) => x.bValue).toList(); // no error, type parameter inferred on closure and map<T>.
}

When dart2js splits a program with these types, it ignores the error cases and allows those deferred types to be deferred. This is unsound in the presence of reified types.

To address performance problems in our current implementation of deferred loading, we are revisiting the algorithm and our changes are likely going to affect this behavior.

4 options so far:

  1. Make this a proper error: change the CFE/analyzer, announce a breaking change, and ignore the issue in dart2js.
  2. Make deferred loading more flexible: the errors above exist because dart2js represents types and classes as one thing. If we split the runtime type representation in dart2js, we could remove this limitation of deferred loading.
  3. Make this a warning and do a sound code split by assigning more types to the main output unit.
  4. Hide the error and try to mimic the current unsound behavior (either by ignoring certain cases or by using the "any" type we have for js-interop)

(2) is our long term goal, but it will take time (at least a few months), (1) seems unfortunate if we are aiming to do (2). (4) could work because today's behavior is working for our users, however we are not sure if it's feasible to do after we change the algorithm implementation (in particular, some patterns we want to ignore are hard to distinguish from problematic patterns like #33046).

@sigmundch

This comment has been minimized.

Member

sigmundch commented Dec 5, 2018

My proposal right now is to go with (3)

Turns out that there was also a bug in the old implementation that prevented constructor elements from being treated as deferred when accessed directly via a deferred import.

Going with a sound approach that fixes this bug seems to have a positive effect on code-split for some large customer apps (0.5% better!). Using an unsound approach, for example ignoring return types in closures, improves the result in that app by maybe 0.05%, so it doesn't appear to be worthwhile.

@sigmundch

This comment has been minimized.

Member

sigmundch commented Dec 5, 2018

fixed by 70e1517

@sigmundch sigmundch closed this Dec 5, 2018

dart-bot pushed a commit that referenced this issue Dec 5, 2018

Record deferred accesses in kernel world impacts and use it for split…
…ting.

On large apps this cuts down the time spent in the deferred loading algorithm by
two thirds. To address issue #35311, this CL keeps things sound and may load in
the main unit more code.

On some large apps, it appears the effect of this change is not that measurable,
and we even see an improvement. I'm still validating the data, but I believe
this is in part because there was a different bug
in the previous algorithm: constructors were never deferred
because we were looking for the constructor-name, usually '', instead of the
enclosing class name.

My current data is that, the main unit of some large app shows:
  old algorithm:        13,213,191
  sound algorithm:      13,150,145
  unsound algorithm*:   13,147,282
  fixed old algorithm:  13,146,509

* ignoring return type of closures


Change-Id: I7d3e525393ef38979b26051b4d354fc1001560af
Reviewed-on: https://dart-review.googlesource.com/c/85725
Commit-Queue: Sigmund Cherem <sigmund@google.com>
Reviewed-by: Johnni Winther <johnniwinther@google.com>

dart-bot pushed a commit that referenced this issue Dec 5, 2018

Revert "Record deferred accesses in kernel world impacts and use it f…
…or splitting."

This reverts commit 70e1517.

Reason for revert: saw unexpected regression on acx-gallery as well. I'd like to look in more detail and reland if it is justified.

Original change's description:
> Record deferred accesses in kernel world impacts and use it for splitting.
> 
> On large apps this cuts down the time spent in the deferred loading algorithm by
> two thirds. To address issue #35311, this CL keeps things sound and may load in
> the main unit more code.
> 
> On some large apps, it appears the effect of this change is not that measurable,
> and we even see an improvement. I'm still validating the data, but I believe
> this is in part because there was a different bug
> in the previous algorithm: constructors were never deferred
> because we were looking for the constructor-name, usually '', instead of the
> enclosing class name.
> 
> My current data is that, the main unit of some large app shows:
>   old algorithm:        13,213,191
>   sound algorithm:      13,150,145
>   unsound algorithm*:   13,147,282
>   fixed old algorithm:  13,146,509
> 
> * ignoring return type of closures
> 
> 
> Change-Id: I7d3e525393ef38979b26051b4d354fc1001560af
> Reviewed-on: https://dart-review.googlesource.com/c/85725
> Commit-Queue: Sigmund Cherem <sigmund@google.com>
> Reviewed-by: Johnni Winther <johnniwinther@google.com>

TBR=johnniwinther@google.com,sigmund@google.com

Change-Id: I6992279bebcc99578f94087a17d6c327b32a0876
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://dart-review.googlesource.com/c/86246
Reviewed-by: Sigmund Cherem <sigmund@google.com>
Commit-Queue: Sigmund Cherem <sigmund@google.com>

@sigmundch sigmundch reopened this Dec 6, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment