Only reload libraries when they may have been modified.

Only reload libraries when they may have been modified.

We now check all of the scripts which make up a library.  If any
script is modified, then we consider that library to be modified.  We
also consider any library which imports a modified library to be
modified.  To propagate this info efficiently, we build and discard
the imported-by graph when beginning a reload.

The embedder must provide a FileModifiedCallback in order to support
the detection of modified scripts.

In order to detect changes to package: libraries, we have modified the
embedder interface to provide the resolved url when possible, so that
we don't need to re-resolve package uris when checking for reload.
This change is incompatible and all embedders will need to be updated.

We now support a "force" flag when reloading sources.  This causes all
libraries to be reloaded regardless of whether the underlying scripts
have been updated.

Closes #26919

R=johnmccutchan@google.com

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

[turnidge, 2016-07-28 22:57:08]

turnidge@google.com changed reviewers:
+ johnmccutchan@google.com

[Cutch, 2016-07-29 14:37:04]

LGTM w/c

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

https://codereview.chromium.org/2186423002/diff/20001/runtime/include/dart_ap...
runtime/include/dart_api.h:2718: DART_EXPORT Dart_Handle
Dart_LoadScript(Dart_Handle url,
update API documentation for these methods?

https://codereview.chromium.org/2186423002/diff/20001/runtime/vm/isolate_relo...
File runtime/vm/isolate_reload.cc (right):

https://codereview.chromium.org/2186423002/diff/20001/runtime/vm/isolate_relo...
runtime/vm/isolate_reload.cc:183: reload_timestamp_(OS::GetCurrentTimeMillis()),
Why does the reload context and the isolate have a reload timestamp? It looks
like you use the isolate's below.

https://codereview.chromium.org/2186423002/diff/20001/runtime/vm/isolate_relo...
runtime/vm/isolate_reload.cc:561: if (ScriptModifiedSince(script, last_reload))
{
maybe merge these two ifs:

if (force_reload || ScriptMOdifiedSince(script, last_reload))

https://codereview.chromium.org/2186423002/diff/20001/runtime/vm/isolate_relo...
runtime/vm/isolate_reload.cc:567: PropagateLibraryModified(imported_by, lib_idx,
modified_libs);
Could this be rolled into the if statement above?

if ... {
  modified_libs->Add(lib_idx);
  PropgateLibraryModified(imported_by, lib_idx, modified_libs);
}

[turnidge, 2016-07-29 17:37:38]

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

https://codereview.chromium.org/2186423002/diff/20001/runtime/include/dart_ap...
runtime/include/dart_api.h:2718: DART_EXPORT Dart_Handle
Dart_LoadScript(Dart_Handle url,
On 2016/07/29 14:37:03, Cutch wrote:
> update API documentation for these methods?

Done, and then some.  I decided to finally write some documentation for the
LibraryTagHandler in general.

https://codereview.chromium.org/2186423002/diff/20001/runtime/vm/isolate_relo...
File runtime/vm/isolate_reload.cc (right):

https://codereview.chromium.org/2186423002/diff/20001/runtime/vm/isolate_relo...
runtime/vm/isolate_reload.cc:183: reload_timestamp_(OS::GetCurrentTimeMillis()),
On 2016/07/29 14:37:04, Cutch wrote:
> Why does the reload context and the isolate have a reload timestamp? It looks
> like you use the isolate's below.

The isolate's reload timestamp is the timestamp of the last successful reload.

The IRC's reload timestamp is the timestamp that *will be* recorded if the
current reload succeeds.  This happens in isolate.cc.

I have renamed the isolate's timestamp to be "last_reload_timestamp" to make the
distinction more clear.  I have also moved setting the last_reload_timestamp
into isolate_reload.cc.

https://codereview.chromium.org/2186423002/diff/20001/runtime/vm/isolate_relo...
runtime/vm/isolate_reload.cc:561: if (ScriptModifiedSince(script, last_reload))
{
On 2016/07/29 14:37:04, Cutch wrote:
> maybe merge these two ifs:
> 
> if (force_reload || ScriptMOdifiedSince(script, last_reload))

Done.

https://codereview.chromium.org/2186423002/diff/20001/runtime/vm/isolate_relo...
runtime/vm/isolate_reload.cc:567: PropagateLibraryModified(imported_by, lib_idx,
modified_libs);
On 2016/07/29 14:37:04, Cutch wrote:
> Could this be rolled into the if statement above?
> 
> if ... {
>   modified_libs->Add(lib_idx);
>   PropgateLibraryModified(imported_by, lib_idx, modified_libs);
> }

Done.

[turnidge, 2016-07-29 17:41:51]

Description was changed from

==========
Only reload libraries when they may have been modified.

We now check all of the scripts which make up a library.  If any
script is modified, then we consider that library to be modified.  We
also consider any library which imports a modified library to be
modified.  To propagate this info efficiently, we build and discard
the imported-by graph when beginning a reload.

The embedder must provide a FileModifiedCallback in order to support
the detection of modified scripts.

In order to detect changes to package: libraries, we have modified the
embedder interface to provide the resolved url when possible, so that
we don't need to re-resolve package uris when checking for reload.
This change is incompatible and all embedders will need to be updated.

We now support a "force" flag when reloading sources.  This causes all
libraries to be reloaded regardless of whether the underlying scripts
have been updated.

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

to

==========
Only reload libraries when they may have been modified.

We now check all of the scripts which make up a library.  If any
script is modified, then we consider that library to be modified.  We
also consider any library which imports a modified library to be
modified.  To propagate this info efficiently, we build and discard
the imported-by graph when beginning a reload.

The embedder must provide a FileModifiedCallback in order to support
the detection of modified scripts.

In order to detect changes to package: libraries, we have modified the
embedder interface to provide the resolved url when possible, so that
we don't need to re-resolve package uris when checking for reload.
This change is incompatible and all embedders will need to be updated.

We now support a "force" flag when reloading sources.  This causes all
libraries to be reloaded regardless of whether the underlying scripts
have been updated.

Closes #26919
==========

[turnidge, 2016-07-29 18:23:22]

Description was changed from

==========
Only reload libraries when they may have been modified.

We now check all of the scripts which make up a library.  If any
script is modified, then we consider that library to be modified.  We
also consider any library which imports a modified library to be
modified.  To propagate this info efficiently, we build and discard
the imported-by graph when beginning a reload.

The embedder must provide a FileModifiedCallback in order to support
the detection of modified scripts.

In order to detect changes to package: libraries, we have modified the
embedder interface to provide the resolved url when possible, so that
we don't need to re-resolve package uris when checking for reload.
This change is incompatible and all embedders will need to be updated.

We now support a "force" flag when reloading sources.  This causes all
libraries to be reloaded regardless of whether the underlying scripts
have been updated.

Closes #26919
==========

to

==========
Only reload libraries when they may have been modified.

We now check all of the scripts which make up a library.  If any
script is modified, then we consider that library to be modified.  We
also consider any library which imports a modified library to be
modified.  To propagate this info efficiently, we build and discard
the imported-by graph when beginning a reload.

The embedder must provide a FileModifiedCallback in order to support
the detection of modified scripts.

In order to detect changes to package: libraries, we have modified the
embedder interface to provide the resolved url when possible, so that
we don't need to re-resolve package uris when checking for reload.
This change is incompatible and all embedders will need to be updated.

We now support a "force" flag when reloading sources.  This causes all
libraries to be reloaded regardless of whether the underlying scripts
have been updated.

Closes #26919

R=johnmccutchan@google.com

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

[turnidge, 2016-07-29 18:23:23]

Committed patchset #3 (id:40001) manually as
1166f83bf6fcad764d7599b9f5e7bd80b242cf25 (presubmit successful).