Precompile dependencies' executables for use with "pub run".

Precompile dependencies' executables for use with "pub run".

R=rnystrom@google.com
BUG=20482
Committed: https://code.google.com/p/dart/source/detail?r=39424

[Bob Nystrom, 2014-08-18 21:31:12]

This came out very well. Two questions:

- How should this interact with pub cache repair?
- How can we test that this actually invokes the snapshot and doesn't just run
the executable from source?

https://codereview.chromium.org/482053002/diff/1/sdk/lib/_internal/pub/lib/sr...
File sdk/lib/_internal/pub/lib/src/entrypoint.dart (right):

https://codereview.chromium.org/482053002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/entrypoint.dart:148: return
precompileExecutables().catchError((error, stackTrace) {
We should add a flag to enable/disable this step. It may be worth making it off
by default at first.

https://codereview.chromium.org/482053002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/entrypoint.dart:161: var executables = new
Map.fromIterable(root.immediateDependencies,
I <3 HoFs, but at this scale, I think an imperative loop over
root.immediateDependencies will be easier to read.

https://codereview.chromium.org/482053002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/entrypoint.dart:172:
graph.lockFile.packages[package.name].source == 'path')) {
Instead of checking for "path", do: is! CachedSource

Double check, but I think that may handle the root package too.

https://codereview.chromium.org/482053002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/entrypoint.dart:205: return
waitAndPrintErrors(executables.keys.map((package) {
How about hoisting this callback to a private method?

https://codereview.chromium.org/482053002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/entrypoint.dart:208: return
environment.servePackageBinDirectory(package).then((server) {
If there are a lot of packages, this may end up binding a bunch of ports. Now is
probably a good time to add support for unserving this to AssetEnvironment, and
then stop serving each package when it's done.

https://codereview.chromium.org/482053002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/entrypoint.dart:219: // TODO(nweiz): Stop manually
deleting this when issue 20504 is
Long line.

https://codereview.chromium.org/482053002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/entrypoint.dart:236: /// Returns the executable
name for [id].
Document that this makes it bold, or that the name is intended for log output.

https://codereview.chromium.org/482053002/diff/1/sdk/lib/_internal/pub/lib/sr...
File sdk/lib/_internal/pub/lib/src/executable.dart (right):

https://codereview.chromium.org/482053002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/executable.dart:148: readTextFile(sdkVersionPath)
== sdk.version.toString()) {
Instead of reading the file, how about putting the SDK in the filename
("sdk-version-1.6.1")? To see if the snapshots are up to date, you just need to
see if that file exists.

To delete an old one, you'll have to walk the directory, but that's not on the
critical perf path.

Actually, you could even put the SDK version in the snapshot's path. Something
like .pub/bin-1.6.1/foo.snapshot or .pub/bin/foo-1.6.1.snapshot. Then you only
need a single existence check.

https://codereview.chromium.org/482053002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/executable.dart:152: log.fine("Precompiled
executables are out of date.");
If users auto-update the SDK (which many do), it means pub run will get
mysteriously, silently slow about once a week. I think we probably need to
message this to users somehow. At the same time, adding extra output to the
executable non-deterministically doesn't seem like a great user experience.

Do you think we need to do anything about this?

https://codereview.chromium.org/482053002/diff/1/sdk/lib/_internal/pub/lib/sr...
File sdk/lib/_internal/pub/lib/src/package.dart (right):

https://codereview.chromium.org/482053002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/package.dart:157: return recursive ||
!file.contains('/');
If recursive is true, I think it would be clearer to just skip the where() call
completely.

https://codereview.chromium.org/482053002/diff/1/sdk/lib/_internal/pub/lib/sr...
File sdk/lib/_internal/pub/lib/src/package_graph.dart (right):

https://codereview.chromium.org/482053002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/package_graph.dart:36: /// includes dev and
override. For any other package, it will ignore dev and
"will return" -> "returns"
"will ignore" -> "ignores"

Present tense is terser. :)

https://codereview.chromium.org/482053002/diff/1/sdk/lib/_internal/pub/lib/sr...
File sdk/lib/_internal/pub/lib/src/utils.dart (right):

https://codereview.chromium.org/482053002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/utils.dart:342: for (var vertex1 in graph.keys) {
How about:

// Add all of the immediate edges to the result.
graph.forEach((vertex, edges) {
  result[vertex] = new Set.from(edges)..add(vertex);
}

https://codereview.chromium.org/482053002/diff/1/sdk/lib/_internal/pub/test/s...
File
sdk/lib/_internal/pub/test/snapshot/doesnt_snapshot_transitive_dependencies_test.dart
(right):

https://codereview.chromium.org/482053002/diff/1/sdk/lib/_internal/pub/test/s...
sdk/lib/_internal/pub/test/snapshot/doesnt_snapshot_transitive_dependencies_test.dart:15:
integration("doesn't create a snapshot for transitive dependencies' "
Test that it does create a snapshot for a dep that is both immediate and
transitive.

https://codereview.chromium.org/482053002/diff/1/sdk/lib/_internal/pub/test/s...
File
sdk/lib/_internal/pub/test/snapshot/prints_errors_for_broken_snapshots_test.dart
(right):

https://codereview.chromium.org/482053002/diff/1/sdk/lib/_internal/pub/test/s...
sdk/lib/_internal/pub/test/snapshot/prints_errors_for_broken_snapshots_test.dart:22:
d.file("goodbye.dart", "void main() {"),
Nit, but can you make this code more obviously wrong?

https://codereview.chromium.org/482053002/diff/1/sdk/lib/_internal/pub/test/s...
File
sdk/lib/_internal/pub/test/snapshot/recompiles_if_the_sdk_is_out_of_date_test.dart
(right):

https://codereview.chromium.org/482053002/diff/1/sdk/lib/_internal/pub/test/s...
sdk/lib/_internal/pub/test/snapshot/recompiles_if_the_sdk_is_out_of_date_test.dart:18:
servePackages([packageMap("foo", "1.2.3")], contents: [
Nit but use a different version number here. It's confusingly similar to the
fake SDK version even though the two aren't related.

[nweiz, 2014-08-19 20:19:47]

> How should this interact with pub cache repair?

I don't think it should have any interaction. Repairing the cache shouldn't
change any of its contents from what they were when they were originally
downloaded, it should just restore them to their pristine state. This will
always run against the pristine state.

Besides, cache repair is a global operation; there's no way for us to know where
all the packages that use the repaired cache are even if we wanted to do
something to their executables.

> How can we test that this actually invokes the snapshot and doesn't just run
> the executable from source?

Since this is a transparent cache, I don't think we can, which is why I didn't
add a test for it. We could maybe try to trick pub into thinking the executable
is up-to-date when it's not, but I don't think that's sufficiently reliable or
useful.

https://codereview.chromium.org/482053002/diff/1/sdk/lib/_internal/pub/lib/sr...
File sdk/lib/_internal/pub/lib/src/entrypoint.dart (right):

https://codereview.chromium.org/482053002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/entrypoint.dart:148: return
precompileExecutables().catchError((error, stackTrace) {
On 2014/08/18 21:31:12, Bob Nystrom wrote:
> We should add a flag to enable/disable this step.

Why? It doesn't take that long relative to "pub get" and it provides a lot of
value. I don't think it's worth adding this flag unless we see users actually
asking for it.

> It may be worth making it off by default at first.

I definitely disagree with this. As soon as this patch lands, the executables it
precompiles will be available efficiently. Given that, it's very nice to have
this provide users with a list of available executables.

https://codereview.chromium.org/482053002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/entrypoint.dart:161: var executables = new
Map.fromIterable(root.immediateDependencies,
On 2014/08/18 21:31:11, Bob Nystrom wrote:
> I <3 HoFs, but at this scale, I think an imperative loop over
> root.immediateDependencies will be easier to read.

I disagree, especially since we want to short-circuit several times in the value
block.

https://codereview.chromium.org/482053002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/entrypoint.dart:172:
graph.lockFile.packages[package.name].source == 'path')) {
On 2014/08/18 21:31:11, Bob Nystrom wrote:
> Instead of checking for "path", do: is! CachedSource
> 
> Double check, but I think that may handle the root package too.

Done.

https://codereview.chromium.org/482053002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/entrypoint.dart:205: return
waitAndPrintErrors(executables.keys.map((package) {
On 2014/08/18 21:31:11, Bob Nystrom wrote:
> How about hoisting this callback to a private method?

Done.

https://codereview.chromium.org/482053002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/entrypoint.dart:208: return
environment.servePackageBinDirectory(package).then((server) {
On 2014/08/18 21:31:12, Bob Nystrom wrote:
> If there are a lot of packages, this may end up binding a bunch of ports. Now
is
> probably a good time to add support for unserving this to AssetEnvironment,
and
> then stop serving each package when it's done.

Added a TODO.

https://codereview.chromium.org/482053002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/entrypoint.dart:219: // TODO(nweiz): Stop manually
deleting this when issue 20504 is
On 2014/08/18 21:31:11, Bob Nystrom wrote:
> Long line.

Done.

https://codereview.chromium.org/482053002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/entrypoint.dart:236: /// Returns the executable
name for [id].
On 2014/08/18 21:31:11, Bob Nystrom wrote:
> Document that this makes it bold, or that the name is intended for log output.

Done.

https://codereview.chromium.org/482053002/diff/1/sdk/lib/_internal/pub/lib/sr...
File sdk/lib/_internal/pub/lib/src/executable.dart (right):

https://codereview.chromium.org/482053002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/executable.dart:148: readTextFile(sdkVersionPath)
== sdk.version.toString()) {
On 2014/08/18 21:31:12, Bob Nystrom wrote:
> Instead of reading the file, how about putting the SDK in the filename
> ("sdk-version-1.6.1")? To see if the snapshots are up to date, you just need
to
> see if that file exists.
> 
> To delete an old one, you'll have to walk the directory, but that's not on the
> critical perf path.
> 
> Actually, you could even put the SDK version in the snapshot's path. Something
> like .pub/bin-1.6.1/foo.snapshot or .pub/bin/foo-1.6.1.snapshot. Then you only
> need a single existence check.

I'd like to wait to do this until we see if this file read causes a noticeable
perf hit. Historically just reading a file has actually been very fast in Dart.
We can change the versioning scheme as you describe in a backwards-compatible
way later if we want.

https://codereview.chromium.org/482053002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/executable.dart:152: log.fine("Precompiled
executables are out of date.");
On 2014/08/18 21:31:12, Bob Nystrom wrote:
> If users auto-update the SDK (which many do), it means pub run will get
> mysteriously, silently slow about once a week. I think we probably need to
> message this to users somehow. At the same time, adding extra output to the
> executable non-deterministically doesn't seem like a great user experience.
> 
> Do you think we need to do anything about this?

When we last talked about this, we decided that it was more important not to
have pub print extra messages here than it was to make it clear why running was
occasionally slow.

https://codereview.chromium.org/482053002/diff/1/sdk/lib/_internal/pub/lib/sr...
File sdk/lib/_internal/pub/lib/src/package.dart (right):

https://codereview.chromium.org/482053002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/package.dart:157: return recursive ||
!file.contains('/');
On 2014/08/18 21:31:12, Bob Nystrom wrote:
> If recursive is true, I think it would be clearer to just skip the where()
call
> completely.

Done.

https://codereview.chromium.org/482053002/diff/1/sdk/lib/_internal/pub/lib/sr...
File sdk/lib/_internal/pub/lib/src/package_graph.dart (right):

https://codereview.chromium.org/482053002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/package_graph.dart:36: /// includes dev and
override. For any other package, it will ignore dev and
On 2014/08/18 21:31:12, Bob Nystrom wrote:
> "will return" -> "returns"
> "will ignore" -> "ignores"
> 
> Present tense is terser. :)

Done.

https://codereview.chromium.org/482053002/diff/1/sdk/lib/_internal/pub/lib/sr...
File sdk/lib/_internal/pub/lib/src/utils.dart (right):

https://codereview.chromium.org/482053002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/utils.dart:342: for (var vertex1 in graph.keys) {
On 2014/08/18 21:31:12, Bob Nystrom wrote:
> How about:
> 
> // Add all of the immediate edges to the result.
> graph.forEach((vertex, edges) {
>   result[vertex] = new Set.from(edges)..add(vertex);
> }

Done.

https://codereview.chromium.org/482053002/diff/1/sdk/lib/_internal/pub/test/s...
File
sdk/lib/_internal/pub/test/snapshot/doesnt_snapshot_transitive_dependencies_test.dart
(right):

https://codereview.chromium.org/482053002/diff/1/sdk/lib/_internal/pub/test/s...
sdk/lib/_internal/pub/test/snapshot/doesnt_snapshot_transitive_dependencies_test.dart:15:
integration("doesn't create a snapshot for transitive dependencies' "
On 2014/08/18 21:31:12, Bob Nystrom wrote:
> Test that it does create a snapshot for a dep that is both immediate and
> transitive.

Done.

https://codereview.chromium.org/482053002/diff/1/sdk/lib/_internal/pub/test/s...
File
sdk/lib/_internal/pub/test/snapshot/prints_errors_for_broken_snapshots_test.dart
(right):

https://codereview.chromium.org/482053002/diff/1/sdk/lib/_internal/pub/test/s...
sdk/lib/_internal/pub/test/snapshot/prints_errors_for_broken_snapshots_test.dart:22:
d.file("goodbye.dart", "void main() {"),
On 2014/08/18 21:31:12, Bob Nystrom wrote:
> Nit, but can you make this code more obviously wrong?

Done.

https://codereview.chromium.org/482053002/diff/1/sdk/lib/_internal/pub/test/s...
File
sdk/lib/_internal/pub/test/snapshot/recompiles_if_the_sdk_is_out_of_date_test.dart
(right):

https://codereview.chromium.org/482053002/diff/1/sdk/lib/_internal/pub/test/s...
sdk/lib/_internal/pub/test/snapshot/recompiles_if_the_sdk_is_out_of_date_test.dart:18:
servePackages([packageMap("foo", "1.2.3")], contents: [
On 2014/08/18 21:31:12, Bob Nystrom wrote:
> Nit but use a different version number here. It's confusingly similar to the
> fake SDK version even though the two aren't related.

Done.

[nweiz, 2014-08-19 20:20:32]

Code review changes

[Bob Nystrom, 2014-08-19 21:22:28]

>> How can we test that this actually invokes the snapshot and doesn't just run
>> the executable from source?
>
> Since this is a transparent cache, I don't think we can, which is why I didn't
> add a test for it.

I guess that makes sense. This does really encourage to get some benchmarks in
place then. Otherwise, we don't have any automated way of telling if this patch
does anything useful.

https://codereview.chromium.org/482053002/diff/1/sdk/lib/_internal/pub/lib/sr...
File sdk/lib/_internal/pub/lib/src/entrypoint.dart (right):

https://codereview.chromium.org/482053002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/entrypoint.dart:148: return
precompileExecutables().catchError((error, stackTrace) {
On 2014/08/19 20:19:46, nweiz wrote:
> On 2014/08/18 21:31:12, Bob Nystrom wrote:
> > We should add a flag to enable/disable this step.
> 
> Why? It doesn't take that long relative to "pub get" and it provides a lot of
> value. I don't think it's worth adding this flag unless we see users actually
> asking for it.
> 
> > It may be worth making it off by default at first.
> 
> I definitely disagree with this. As soon as this patch lands, the executables
it
> precompiles will be available efficiently. Given that, it's very nice to have
> this provide users with a list of available executables.

OK, SGTM.

https://codereview.chromium.org/482053002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/entrypoint.dart:161: var executables = new
Map.fromIterable(root.immediateDependencies,
On 2014/08/19 20:19:46, nweiz wrote:
> On 2014/08/18 21:31:11, Bob Nystrom wrote:
> > I <3 HoFs, but at this scale, I think an imperative loop over
> > root.immediateDependencies will be easier to read.
> 
> I disagree, especially since we want to short-circuit several times in the
value
> block.

The "continue" statement will short-circuit in a loop. :)

I'm pretty comfortable with functional style code, but this blob is pretty big
and nasty.

https://codereview.chromium.org/482053002/diff/1/sdk/lib/_internal/pub/lib/sr...
File sdk/lib/_internal/pub/lib/src/executable.dart (right):

https://codereview.chromium.org/482053002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/executable.dart:148: readTextFile(sdkVersionPath)
== sdk.version.toString()) {
On 2014/08/19 20:19:46, nweiz wrote:
> On 2014/08/18 21:31:12, Bob Nystrom wrote:
> > Instead of reading the file, how about putting the SDK in the filename
> > ("sdk-version-1.6.1")? To see if the snapshots are up to date, you just need
> to
> > see if that file exists.
> > 
> > To delete an old one, you'll have to walk the directory, but that's not on
the
> > critical perf path.
> > 
> > Actually, you could even put the SDK version in the snapshot's path.
Something
> > like .pub/bin-1.6.1/foo.snapshot or .pub/bin/foo-1.6.1.snapshot. Then you
only
> > need a single existence check.
> 
> I'd like to wait to do this until we see if this file read causes a noticeable
> perf hit. Historically just reading a file has actually been very fast in
Dart.
> We can change the versioning scheme as you describe in a backwards-compatible
> way later if we want.

Is there a reason to wait on this? No matter what, one file stat will be faster
than two. Is there a reason to prefer the latter?

https://codereview.chromium.org/482053002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/executable.dart:152: log.fine("Precompiled
executables are out of date.");
On 2014/08/19 20:19:46, nweiz wrote:
> On 2014/08/18 21:31:12, Bob Nystrom wrote:
> > If users auto-update the SDK (which many do), it means pub run will get
> > mysteriously, silently slow about once a week. I think we probably need to
> > message this to users somehow. At the same time, adding extra output to the
> > executable non-deterministically doesn't seem like a great user experience.
> > 
> > Do you think we need to do anything about this?
> 
> When we last talked about this, we decided that it was more important not to
> have pub print extra messages here than it was to make it clear why running
was
> occasionally slow.

SGTM.

https://codereview.chromium.org/482053002/diff/20001/sdk/lib/_internal/pub/li...
File sdk/lib/_internal/pub/lib/src/entrypoint.dart (right):

https://codereview.chromium.org/482053002/diff/20001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/entrypoint.dart:216: Future
_precompileExecutablesForPackage(
Doc comment.

[nweiz, 2014-08-19 21:46:40]

> I guess that makes sense. This does really encourage to get some benchmarks in
> place then. Otherwise, we don't have any automated way of telling if this
patch
> does anything useful.

I'm not that hardcore about automatically validating everything every patch
does. We know this does that it wants to, and we're not likely to break that
aspect of it. It might be nice to have a benchmark to track the speed
improvement of cached "pub run" from 300ms down, but that seems tangential to
this to me.

https://codereview.chromium.org/482053002/diff/1/sdk/lib/_internal/pub/lib/sr...
File sdk/lib/_internal/pub/lib/src/entrypoint.dart (right):

https://codereview.chromium.org/482053002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/entrypoint.dart:161: var executables = new
Map.fromIterable(root.immediateDependencies,
On 2014/08/19 21:22:28, Bob Nystrom wrote:
> On 2014/08/19 20:19:46, nweiz wrote:
> > On 2014/08/18 21:31:11, Bob Nystrom wrote:
> > > I <3 HoFs, but at this scale, I think an imperative loop over
> > > root.immediateDependencies will be easier to read.
> > 
> > I disagree, especially since we want to short-circuit several times in the
> value
> > block.
> 
> The "continue" statement will short-circuit in a loop. :)
> 
> I'm pretty comfortable with functional style code, but this blob is pretty big
> and nasty.

I've moved it out into its own function.

https://codereview.chromium.org/482053002/diff/1/sdk/lib/_internal/pub/lib/sr...
File sdk/lib/_internal/pub/lib/src/executable.dart (right):

https://codereview.chromium.org/482053002/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/executable.dart:148: readTextFile(sdkVersionPath)
== sdk.version.toString()) {
On 2014/08/19 21:22:28, Bob Nystrom wrote:
> On 2014/08/19 20:19:46, nweiz wrote:
> > On 2014/08/18 21:31:12, Bob Nystrom wrote:
> > > Instead of reading the file, how about putting the SDK in the filename
> > > ("sdk-version-1.6.1")? To see if the snapshots are up to date, you just
need
> > to
> > > see if that file exists.
> > > 
> > > To delete an old one, you'll have to walk the directory, but that's not on
> the
> > > critical perf path.
> > > 
> > > Actually, you could even put the SDK version in the snapshot's path.
> Something
> > > like .pub/bin-1.6.1/foo.snapshot or .pub/bin/foo-1.6.1.snapshot. Then you
> only
> > > need a single existence check.
> > 
> > I'd like to wait to do this until we see if this file read causes a
noticeable
> > perf hit. Historically just reading a file has actually been very fast in
> Dart.
> > We can change the versioning scheme as you describe in a
backwards-compatible
> > way later if we want.
> 
> Is there a reason to wait on this? No matter what, one file stat will be
faster
> than two. Is there a reason to prefer the latter?

It adds a fair amount of complexity, especially to the code in pub get that
validates whether the SDK is up-to-date.

https://codereview.chromium.org/482053002/diff/20001/sdk/lib/_internal/pub/li...
File sdk/lib/_internal/pub/lib/src/entrypoint.dart (right):

https://codereview.chromium.org/482053002/diff/20001/sdk/lib/_internal/pub/li...
sdk/lib/_internal/pub/lib/src/entrypoint.dart:216: Future
_precompileExecutablesForPackage(
On 2014/08/19 21:22:28, Bob Nystrom wrote:
> Doc comment.

Done.

[nweiz, 2014-08-19 21:47:03]

Code review changes

[Bob Nystrom, 2014-08-19 22:30:17]

lgtm

[nweiz, 2014-08-20 20:06:33]

Committed patchset #4 manually as 39424 (presubmit successful).