Reload native extensions when starting from a snapshot.

Reload native extensions when starting from a snapshot.

Fixes #21180
Fixes #27574

R=johnmccutchan@google.com

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

[rmacnak, 2016-11-02 22:05:19]

rmacnak@google.com changed reviewers:
+ asiva@google.com, johnmccutchan@google.com

[siva, 2016-11-02 23:48:38]

https://codereview.chromium.org/2475523002/diff/40001/runtime/bin/dartutils.h
File runtime/bin/dartutils.h (right):

https://codereview.chromium.org/2475523002/diff/40001/runtime/bin/dartutils.h...
runtime/bin/dartutils.h:128: static char* RemoveFilename(const char* url);
Would File::DirName be a more appropriate implementation instead
of RemoveFilename here?

https://codereview.chromium.org/2475523002/diff/40001/runtime/bin/loader.cc
File runtime/bin/loader.cc (right):

https://codereview.chromium.org/2475523002/diff/40001/runtime/bin/loader.cc#n...
runtime/bin/loader.cc:476: &library_id);
You could use Dart_ListGetRange to extract the library objects into
an array of handles, that way the expensive Dart_ListGetAt does not
run on every iteration of the loop.

https://codereview.chromium.org/2475523002/diff/40001/runtime/bin/loader.cc#n...
runtime/bin/loader.cc:488: &import_id);
Ditto comment about Dart_ListGetAt

https://codereview.chromium.org/2475523002/diff/40001/runtime/bin/loader.cc#n...
runtime/bin/loader.cc:496: if (strncmp(extension_uri, "dart-ext:", 9) == 0) {
magic numbers 9 , 7 etc. use named constants.

https://codereview.chromium.org/2475523002/diff/40001/runtime/bin/loader.cc#n...
runtime/bin/loader.cc:521: }
This seems like an expensive loop we have to run through always
irrespective of whether an extension exists of not.

How about adding a new API method

Dart_Handle Dart_GetExtensions();
that returns a list of tuples
<library, extension URL> of the extensions that have been loaded
into a library.

[Cutch, 2016-11-03 17:45:34]

https://codereview.chromium.org/2475523002/diff/40001/runtime/bin/dartutils.h
File runtime/bin/dartutils.h (right):

https://codereview.chromium.org/2475523002/diff/40001/runtime/bin/dartutils.h...
runtime/bin/dartutils.h:128: static char* RemoveFilename(const char* url);
On 2016/11/02 23:48:38, siva wrote:
> Would File::DirName be a more appropriate implementation instead
> of RemoveFilename here?

+1.

https://codereview.chromium.org/2475523002/diff/40001/runtime/bin/loader.cc
File runtime/bin/loader.cc (right):

https://codereview.chromium.org/2475523002/diff/40001/runtime/bin/loader.cc#n...
runtime/bin/loader.cc:496: if (strncmp(extension_uri, "dart-ext:", 9) == 0) {
On 2016/11/02 23:48:38, siva wrote:
> magic numbers 9 , 7 etc. use named constants.

Also, these schemes already exist: DartUtils::kDartExtensionScheme

and there is already a function:

bool DartUtils::IsDartExtensionSchemeURL(const char* url);

https://codereview.chromium.org/2475523002/diff/40001/runtime/bin/loader.cc#n...
runtime/bin/loader.cc:521: }
On 2016/11/02 23:48:38, siva wrote:
> This seems like an expensive loop we have to run through always
> irrespective of whether an extension exists of not.
> 
> How about adding a new API method
> 
> Dart_Handle Dart_GetExtensions();
> that returns a list of tuples
> <library, extension URL> of the extensions that have been loaded
> into a library.

I agree with the spirit, but, extensions are technically part of the embedder
and the scheme (dart-ext: is not standardized).

https://codereview.chromium.org/2475523002/diff/40001/samples/sample_extensio...
File samples/sample_extension/test/sample_extension_script_snapshot_test.dart
(right):

https://codereview.chromium.org/2475523002/diff/40001/samples/sample_extensio...
samples/sample_extension/test/sample_extension_script_snapshot_test.dart:64:
ProcessResult result = await Process.run(Platform.executable,
the only difference between this file and the previous one is that arguments
passed to Process.run ?

Can this be refactored?

[siva, 2016-11-03 17:53:01]

https://codereview.chromium.org/2475523002/diff/40001/runtime/bin/loader.cc
File runtime/bin/loader.cc (right):

https://codereview.chromium.org/2475523002/diff/40001/runtime/bin/loader.cc#n...
runtime/bin/loader.cc:521: }
On 2016/11/03 17:45:34, Cutch wrote:
> On 2016/11/02 23:48:38, siva wrote:
> > This seems like an expensive loop we have to run through always
> > irrespective of whether an extension exists of not.
> > 
> > How about adding a new API method
> > 
> > Dart_Handle Dart_GetExtensions();
> > that returns a list of tuples
> > <library, extension URL> of the extensions that have been loaded
> > into a library.
> 
> I agree with the spirit, but, extensions are technically part of the embedder
> and the scheme (dart-ext: is not standardized).

Agree, maybe the API could be something like
Dart_Handle Dart_GetLibrariesMatchingScheme(Dart_Handle scheme)

[rmacnak, 2016-11-03 21:05:05]

Added Dart_GetImportsOfScheme.
Rename RemoveFilename to DirName.
Using Dart_ListGetRange.
Split out test helper.

[Cutch, 2016-11-07 22:24:16]

LGTM w/c

https://codereview.chromium.org/2475523002/diff/80001/runtime/include/dart_api.h
File runtime/include/dart_api.h (right):

https://codereview.chromium.org/2475523002/diff/80001/runtime/include/dart_ap...
runtime/include/dart_api.h:2945: * Returns a list where each element 2n is an
importing library and each element
How about:

Returns a flattened list of pairs. The first element in each pair is the
importing library and the second element in each pair is the imported
library....

[rmacnak, 2016-11-08 00:17:32]

https://codereview.chromium.org/2475523002/diff/80001/runtime/include/dart_api.h
File runtime/include/dart_api.h (right):

https://codereview.chromium.org/2475523002/diff/80001/runtime/include/dart_ap...
runtime/include/dart_api.h:2945: * Returns a list where each element 2n is an
importing library and each element
On 2016/11/07 22:24:16, Cutch wrote:
> How about:
> 
> Returns a flattened list of pairs. The first element in each pair is the
> importing library and the second element in each pair is the imported
> library....

Done.

[rmacnak, 2016-11-08 00:17:52]

Description was changed from

==========
Reload native extensions when starting from a snapshot.

Fixes #21180
Fixes #27574
==========

to

==========
Reload native extensions when starting from a snapshot.

Fixes #21180
Fixes #27574

R=johnmccutchan@google.com

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

[rmacnak, 2016-11-08 00:17:53]

Committed patchset #6 (id:100001) manually as
987165f022e868d111b2393c74bb9383ccf87c12 (presubmit successful).