Eliminate multi-callback structure for LibraryLoader.

Eliminate multi-callback structure for LibraryLoader.

BUG=
R=sigmund@google.com

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

[Emily Fortuna, 2017-03-21 17:14:26]

Patchset #1 (id:1) has been deleted

[Emily Fortuna, 2017-03-24 01:03:31]

Patchset #1 (id:20001) has been deleted

[Emily Fortuna, 2017-03-24 01:03:40]

Patchset #1 (id:40001) has been deleted

[Emily Fortuna, 2017-03-24 01:03:48]

Patchset #1 (id:60001) has been deleted

[Emily Fortuna, 2017-03-24 01:03:55]

Patchset #1 (id:80001) has been deleted

[Emily Fortuna, 2017-03-24 01:04:25]

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

[Siggi Cherem (dart-lang), 2017-03-24 17:40:28]

lgtm! woohoo!

Only minor suggestions below.

https://codereview.chromium.org/2760923004/diff/100001/pkg/compiler/lib/src/c...
File pkg/compiler/lib/src/compiler.dart (right):

https://codereview.chromium.org/2760923004/diff/100001/pkg/compiler/lib/src/c...
pkg/compiler/lib/src/compiler.dart:10: import 'io/source_file.dart' show
SourceFile;
nit: move this further down to keep the alphabetical order

https://codereview.chromium.org/2760923004/diff/100001/pkg/compiler/lib/src/c...
pkg/compiler/lib/src/compiler.dart:556: if (loadedLibraries != null) {
minor nit - we tend to prefer the fast-exit pattern that was here before:

if (loadedLibraries == null) return null;

processLoadedLIbraries(loadedLibraries);
...
return library;

https://codereview.chromium.org/2760923004/diff/100001/pkg/compiler/lib/src/c...
pkg/compiler/lib/src/compiler.dart:558: LibraryElement library =
loadedLibraries.rootLibrary;
is it safe to say that the root library is not null at this point, or do we need
to check for that too?

https://codereview.chromium.org/2760923004/diff/100001/pkg/compiler/lib/src/k...
File pkg/compiler/lib/src/kernel/kernel.dart (right):

https://codereview.chromium.org/2760923004/diff/100001/pkg/compiler/lib/src/k...
pkg/compiler/lib/src/kernel/kernel.dart:146: Future<ir.Library> loadLibrary(Uri
uri) async {
it appears this method is never called, so we can probably delete it and remove
the dependency above :)

https://codereview.chromium.org/2760923004/diff/100001/pkg/compiler/lib/src/l...
File pkg/compiler/lib/src/library_loader.dart (left):

https://codereview.chromium.org/2760923004/diff/100001/pkg/compiler/lib/src/l...
pkg/compiler/lib/src/library_loader.dart:551: return
listener.onLibraryScanned(library, handler);
woo hoo!

https://codereview.chromium.org/2760923004/diff/100001/pkg/compiler/lib/src/l...
File pkg/compiler/lib/src/library_loader.dart (right):

https://codereview.chromium.org/2760923004/diff/100001/pkg/compiler/lib/src/l...
pkg/compiler/lib/src/library_loader.dart:159: /// `loadedLibraries.rootLibrary`
nit: add a `.` at the end.

https://codereview.chromium.org/2760923004/diff/100001/pkg/compiler/lib/src/l...
pkg/compiler/lib/src/library_loader.dart:350: // TODO(efortuna): Refactor
mock_compiler to not do this.
thanks for documenting so nicely the issue :)

I have a couple ideas we can try, but we can totally delay these for a separate
CL:
 - idea 1: make this field non-final and public, then the mock compiler can set
it directly and we can set it's default implementation in a way that allows us
to remove the extra indirection with apiimpl.

 - idea 2: make patchLocations and platofmConfigUri a parameter of the loader
task instead. The algorithm in the mock-compiler is not that different than what
we have here, so if we pass a special value for configUri and the map, we might
have something clean to do.

 - idea 3: make the mock compiler use a different path for the PATCH_CORE, to
match exactly what's expected in the map we use here.


I like #2 or #3 - if #3 is simple enough, I'd go with that. If we weren't moving
to load from kernel, I'd make the compiler more configurable about where
patch-files are with #2, but seems unnecessary at this moment.

https://codereview.chromium.org/2760923004/diff/100001/pkg/compiler/lib/src/l...
pkg/compiler/lib/src/library_loader.dart:468: {bool skipFileWithPartOfTag:
false}) async {
seems like you don't need async here

https://codereview.chromium.org/2760923004/diff/100001/pkg/compiler/lib/src/l...
pkg/compiler/lib/src/library_loader.dart:470: LibraryDependencyHandler loader =
new LibraryDependencyHandler(this);
nit: maybe rename loader as handler?

https://codereview.chromium.org/2760923004/diff/100001/pkg/compiler/lib/src/l...
pkg/compiler/lib/src/library_loader.dart:474: if (library == null) {
style nit: we don't use the {} on this fast-exit code, just:

  if (library == null) return null;

https://codereview.chromium.org/2760923004/diff/100001/pkg/compiler/lib/src/l...
pkg/compiler/lib/src/library_loader.dart:762: if (element.isPlatformLibrary &&
so nice to see patching done while loading, yay!

Minor suggestion: consider adding a tiny method with this logic to make it
self-documenting. 

  await patchLibraryIfNecessary(handler, element);

https://codereview.chromium.org/2760923004/diff/100001/pkg/compiler/lib/src/l...
pkg/compiler/lib/src/library_loader.dart:1556: Token scanFile(SourceFile file);
looks like you don't need scanFile after all? could we remove it here and in
compiler.dart?

https://codereview.chromium.org/2760923004/diff/100001/tests/compiler/dart2js...
File tests/compiler/dart2js/library_resolution_test.dart (right):

https://codereview.chromium.org/2760923004/diff/100001/tests/compiler/dart2js...
tests/compiler/dart2js/library_resolution_test.dart:76:
checkLibrary(LoadedLibraries libraries) {
checkLibrary => checkLibraries?

https://codereview.chromium.org/2760923004/diff/100001/tests/compiler/dart2js...
File tests/compiler/dart2js/memory_compiler.dart (right):

https://codereview.chromium.org/2760923004/diff/100001/tests/compiler/dart2js...
tests/compiler/dart2js/memory_compiler.dart:211: final loader = null;
delete?

[Emily Fortuna, 2017-03-24 18:30:20]

https://codereview.chromium.org/2760923004/diff/100001/pkg/compiler/lib/src/c...
File pkg/compiler/lib/src/compiler.dart (right):

https://codereview.chromium.org/2760923004/diff/100001/pkg/compiler/lib/src/c...
pkg/compiler/lib/src/compiler.dart:10: import 'io/source_file.dart' show
SourceFile;
On 2017/03/24 17:40:27, Siggi Cherem (dart-lang) wrote:
> nit: move this further down to keep the alphabetical order

removed

https://codereview.chromium.org/2760923004/diff/100001/pkg/compiler/lib/src/c...
pkg/compiler/lib/src/compiler.dart:556: if (loadedLibraries != null) {
On 2017/03/24 17:40:27, Siggi Cherem (dart-lang) wrote:
> minor nit - we tend to prefer the fast-exit pattern that was here before:
> 
> if (loadedLibraries == null) return null;
> 
> processLoadedLIbraries(loadedLibraries);
> ...
> return library;

Done.

https://codereview.chromium.org/2760923004/diff/100001/pkg/compiler/lib/src/c...
pkg/compiler/lib/src/compiler.dart:558: LibraryElement library =
loadedLibraries.rootLibrary;
On 2017/03/24 17:40:27, Siggi Cherem (dart-lang) wrote:
> is it safe to say that the root library is not null at this point, or do we
need
> to check for that too?

As the code is currently written if you receive a LoadedLibraries object, its
rootLibrary cannot be null. I'll add an assert in the constructor though.

https://codereview.chromium.org/2760923004/diff/100001/pkg/compiler/lib/src/k...
File pkg/compiler/lib/src/kernel/kernel.dart (right):

https://codereview.chromium.org/2760923004/diff/100001/pkg/compiler/lib/src/k...
pkg/compiler/lib/src/kernel/kernel.dart:146: Future<ir.Library> loadLibrary(Uri
uri) async {
On 2017/03/24 17:40:27, Siggi Cherem (dart-lang) wrote:
> it appears this method is never called, so we can probably delete it and
remove
> the dependency above :)

ah good point!

https://codereview.chromium.org/2760923004/diff/100001/pkg/compiler/lib/src/l...
File pkg/compiler/lib/src/library_loader.dart (right):

https://codereview.chromium.org/2760923004/diff/100001/pkg/compiler/lib/src/l...
pkg/compiler/lib/src/library_loader.dart:159: /// `loadedLibraries.rootLibrary`
On 2017/03/24 17:40:28, Siggi Cherem (dart-lang) wrote:
> nit: add a `.` at the end.

Done.

https://codereview.chromium.org/2760923004/diff/100001/pkg/compiler/lib/src/l...
pkg/compiler/lib/src/library_loader.dart:350: // TODO(efortuna): Refactor
mock_compiler to not do this.
On 2017/03/24 17:40:27, Siggi Cherem (dart-lang) wrote:
> thanks for documenting so nicely the issue :)
> 
> I have a couple ideas we can try, but we can totally delay these for a
separate
> CL:
>  - idea 1: make this field non-final and public, then the mock compiler can
set
> it directly and we can set it's default implementation in a way that allows us
> to remove the extra indirection with apiimpl.
> 
>  - idea 2: make patchLocations and platofmConfigUri a parameter of the loader
> task instead. The algorithm in the mock-compiler is not that different than
what
> we have here, so if we pass a special value for configUri and the map, we
might
> have something clean to do.
> 
>  - idea 3: make the mock compiler use a different path for the PATCH_CORE, to
> match exactly what's expected in the map we use here.
> 
> 
> I like #2 or #3 - if #3 is simple enough, I'd go with that. If we weren't
moving
> to load from kernel, I'd make the compiler more configurable about where
> patch-files are with #2, but seems unnecessary at this moment.

makes sense. upcoming CL!

https://codereview.chromium.org/2760923004/diff/100001/pkg/compiler/lib/src/l...
pkg/compiler/lib/src/library_loader.dart:468: {bool skipFileWithPartOfTag:
false}) async {
On 2017/03/24 17:40:28, Siggi Cherem (dart-lang) wrote:
> seems like you don't need async here

Done.

https://codereview.chromium.org/2760923004/diff/100001/pkg/compiler/lib/src/l...
pkg/compiler/lib/src/library_loader.dart:470: LibraryDependencyHandler loader =
new LibraryDependencyHandler(this);
On 2017/03/24 17:40:27, Siggi Cherem (dart-lang) wrote:
> nit: maybe rename loader as handler?

Done.

https://codereview.chromium.org/2760923004/diff/100001/pkg/compiler/lib/src/l...
pkg/compiler/lib/src/library_loader.dart:474: if (library == null) {
On 2017/03/24 17:40:28, Siggi Cherem (dart-lang) wrote:
> style nit: we don't use the {} on this fast-exit code, just:
> 
>   if (library == null) return null;

Done.

https://codereview.chromium.org/2760923004/diff/100001/pkg/compiler/lib/src/l...
pkg/compiler/lib/src/library_loader.dart:762: if (element.isPlatformLibrary &&
On 2017/03/24 17:40:28, Siggi Cherem (dart-lang) wrote:
> so nice to see patching done while loading, yay!
> 
> Minor suggestion: consider adding a tiny method with this logic to make it
> self-documenting. 
> 
>   await patchLibraryIfNecessary(handler, element);

done! I aaaaaalmost did that in my cleanup process, but then I was like "nah".
Done now!

https://codereview.chromium.org/2760923004/diff/100001/pkg/compiler/lib/src/l...
pkg/compiler/lib/src/library_loader.dart:1556: Token scanFile(SourceFile file);
On 2017/03/24 17:40:27, Siggi Cherem (dart-lang) wrote:
> looks like you don't need scanFile after all? could we remove it here and in
> compiler.dart?

yes thanks.

https://codereview.chromium.org/2760923004/diff/100001/tests/compiler/dart2js...
File tests/compiler/dart2js/library_resolution_test.dart (right):

https://codereview.chromium.org/2760923004/diff/100001/tests/compiler/dart2js...
tests/compiler/dart2js/library_resolution_test.dart:76:
checkLibrary(LoadedLibraries libraries) {
On 2017/03/24 17:40:28, Siggi Cherem (dart-lang) wrote:
> checkLibrary => checkLibraries?

Done.

[Emily Fortuna, 2017-03-24 18:30:58]

Patchset #2 (id:120001) has been deleted

[Emily Fortuna, 2017-03-24 19:20:51]

Description was changed from

==========
Eliminate multi-callback structure for LibraryLoader.

BUG=
==========

to

==========
Eliminate multi-callback structure for LibraryLoader.

BUG=
R=sigmund@google.com

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

[Emily Fortuna, 2017-03-24 19:20:52]

Committed patchset #1 (id:100001) manually as
4156ca6a2b3338849136a8e2b3646aab66a9c7aa (presubmit successful).