Don't cache path Context based on cwd, as cwd involves a system-call to compute.

Don't cache path Context based on cwd, as cwd involves a system-call to compute.

Instead, delay evaluation of cwd to the actual use of it.

BUG=
R=rnystrom@google.com

Committed: https://code.google.com/p/dart/source/detail?r=38844

[Anders Johnsen, 2014-07-30 10:19:43]

This speeds up most of the common operations (basename, extension, etc) by a
factor of 10x.

[Bob Nystrom, 2014-07-30 16:15:08]

I really like this change, both the effect and the implementation.

However, it's technically a breaking change. Consider:

var context = new Context();
// Change CWD in OS from "old" to "new".
print(context.current);

In the previous code, this will (intentionally) print "old". With your change,
it prints "new".

In the current code, Context is immutable. If you don't give it a current
directory, it takes a snapshot of it at construction time and preserves that.
This patch changes that.

I'm not opposed to having a mutable context, but I don't want to bump path to
2.0.0. I think the simplest fix is to keep the current public constructor
caching current. Instead, add a private constructor like Context._uncached()
that leaves _current null.

Add a top-level function to context.dart like:

  Context createInternal() => new Context._internal();

Then have path.dart not export that:

  export 'src/context.dart' hide createInternal;

That keeps the public behavior the same but avoids looking up the cwd on
top-level functions that don't need it.

https://codereview.chromium.org/429173002/diff/1/pkg/path/lib/src/context.dart
File pkg/path/lib/src/context.dart (right):

https://codereview.chromium.org/429173002/diff/1/pkg/path/lib/src/context.dar...
pkg/path/lib/src/context.dart:45: /// The current given when Context was
created. If null, current should be
"current" -> "current directory".

"should be" -> "is".

https://codereview.chromium.org/429173002/diff/1/pkg/path/lib/src/context.dar...
pkg/path/lib/src/context.dart:49: /// The current directory that relative paths
will be relative to.
"will be" -> "are".

[nweiz, 2014-07-30 19:27:14]

Don't forget to add a CHANGELOG entry for this.

https://codereview.chromium.org/429173002/diff/1/pkg/path/pubspec.yaml
File pkg/path/pubspec.yaml (right):

https://codereview.chromium.org/429173002/diff/1/pkg/path/pubspec.yaml#newcode2
pkg/path/pubspec.yaml:2: version: 1.2.2+1
This should be 1.2.3. A non-user-visible change to a post-1.0.0 package should
bump the patch number, not the build identifier.

[Anders Johnsen, 2014-07-31 06:19:42]

Good catch Bob, I've changed it. I did create is as a constructor, as creating a
private constructor and then just expose it through a top-level method was a bit
odd :)

PTAL

https://codereview.chromium.org/429173002/diff/1/pkg/path/lib/src/context.dart
File pkg/path/lib/src/context.dart (right):

https://codereview.chromium.org/429173002/diff/1/pkg/path/lib/src/context.dar...
pkg/path/lib/src/context.dart:45: /// The current given when Context was
created. If null, current should be
On 2014/07/30 16:15:08, Bob Nystrom wrote:
> "current" -> "current directory".
> 
> "should be" -> "is".

Done.

https://codereview.chromium.org/429173002/diff/1/pkg/path/lib/src/context.dar...
pkg/path/lib/src/context.dart:49: /// The current directory that relative paths
will be relative to.
On 2014/07/30 16:15:08, Bob Nystrom wrote:
> "will be" -> "are".

Done.

https://codereview.chromium.org/429173002/diff/1/pkg/path/pubspec.yaml
File pkg/path/pubspec.yaml (right):

https://codereview.chromium.org/429173002/diff/1/pkg/path/pubspec.yaml#newcode2
pkg/path/pubspec.yaml:2: version: 1.2.2+1
On 2014/07/30 19:27:13, nweiz wrote:
> This should be 1.2.3. A non-user-visible change to a post-1.0.0 package should
> bump the patch number, not the build identifier.

Done.

[Bob Nystrom, 2014-07-31 15:42:20]

On 2014/07/31 06:19:42, Anders Johnsen wrote:
> Good catch Bob, I've changed it. I did create is as a constructor, as creating
a
> private constructor and then just expose it through a top-level method was a
bit
> odd :)

It's a bit odd, but it keeps that constructor truly private to the package.
We've used the pattern in other packages and I think it works well. We do have
library privacy and export controls (show/hide), so I think we may as well use
them.

[nweiz, 2014-07-31 19:30:24]

I personally prefer the style Anders is using here.

[Bob Nystrom, 2014-07-31 23:17:44]

On 2014/07/31 19:30:24, nweiz wrote:
> I personally prefer the style Anders is using here.

I know. :(

But it's like one line of code and keeps something internal *actually* private.
That seems better to me that having a public method appear in the API docs only
to say "don't use this!".

- bob

[Bob Nystrom, 2014-08-01 16:32:07]

I'd prefer making the internal constructor actually private, but LGTM.

https://codereview.chromium.org/429173002/diff/20001/pkg/path/lib/src/context...
File pkg/path/lib/src/context.dart (right):

https://codereview.chromium.org/429173002/diff/20001/pkg/path/lib/src/context...
pkg/path/lib/src/context.dart:45: Context.internal() : style = Style.platform;
Since _current is final, don't you need to initialize it to null here?

[Anders Johnsen, 2014-08-04 06:33:14]

Did the change, landing :)

https://codereview.chromium.org/429173002/diff/20001/pkg/path/lib/src/context...
File pkg/path/lib/src/context.dart (right):

https://codereview.chromium.org/429173002/diff/20001/pkg/path/lib/src/context...
pkg/path/lib/src/context.dart:45: Context.internal() : style = Style.platform;
On 2014/08/01 16:32:07, Bob Nystrom wrote:
> Since _current is final, don't you need to initialize it to null here?

It's auto-initialized to null.

[Anders Johnsen, 2014-08-04 06:40:25]

Committed patchset #3 manually as 38844 (presubmit successful).

[Anders Johnsen, 2014-08-04 06:58:08]

https://codereview.chromium.org/429173002/diff/20001/pkg/path/lib/src/context...
File pkg/path/lib/src/context.dart (right):

https://codereview.chromium.org/429173002/diff/20001/pkg/path/lib/src/context...
pkg/path/lib/src/context.dart:45: Context.internal() : style = Style.platform;
On 2014/08/04 06:33:14, Anders Johnsen wrote:
> On 2014/08/01 16:32:07, Bob Nystrom wrote:
> > Since _current is final, don't you need to initialize it to null here?
> 
> It's auto-initialized to null.

Ha, nice one Anders... You are of cause right - fixed in followup :)

[Bob Nystrom, 2014-08-04 17:03:46]

On 2014/08/04 06:58:08, Anders Johnsen wrote:
>
https://codereview.chromium.org/429173002/diff/20001/pkg/path/lib/src/context...
> File pkg/path/lib/src/context.dart (right):
> 
>
https://codereview.chromium.org/429173002/diff/20001/pkg/path/lib/src/context...
> pkg/path/lib/src/context.dart:45: Context.internal() : style = Style.platform;
> On 2014/08/04 06:33:14, Anders Johnsen wrote:
> > On 2014/08/01 16:32:07, Bob Nystrom wrote:
> > > Since _current is final, don't you need to initialize it to null here?
> > 
> > It's auto-initialized to null.
> 
> Ha, nice one Anders... You are of cause right - fixed in followup :)

:)

- bob