Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(199)

Issue 1473563003: Cache the current working directory. (Closed)

Created:
5 years ago by nweiz
Modified:
5 years ago
CC:
reviews_dartlang.org
Base URL:
git@github.com:dart-lang/path@master
Target Ref:
refs/heads/master
Visibility:
Public.

Description

Cache the current working directory. Recomputing this isn't terribly expensive, but it happens a lot so it's nice to avoid. Also avoid calling it in relative() when the path is already relative. This brings current from about 0.070 iterations/us to about 0.097, and relative from about 0.080 to about 1.589. R=rnystrom@google.com Committed: https://github.com/dart-lang/path/commit/414bb1e1b7928090b88386c5f15f8ddee43fe96b

Patch Set 1 #

Total comments: 4

Patch Set 2 : Code review changes #

Total comments: 3
Unified diffs Side-by-side diffs Delta from patch set Stats (+29 lines, -3 lines) Patch
M benchmark/benchmark.dart View 1 1 chunk +4 lines, -0 lines 0 comments Download
M lib/path.dart View 1 1 chunk +22 lines, -3 lines 3 comments Download
M lib/src/context.dart View 1 chunk +3 lines, -0 lines 0 comments Download

Messages

Total messages: 12 (3 generated)
nweiz
5 years ago (2015-11-24 06:41:20 UTC) #1
Bob Nystrom
Benchmark? Numbers? https://codereview.chromium.org/1473563003/diff/1/lib/path.dart File lib/path.dart (right): https://codereview.chromium.org/1473563003/diff/1/lib/path.dart#newcode82 lib/path.dart:82: if (uri == _oldUriBase) return _oldCurrent; Document ...
5 years ago (2015-11-24 17:25:16 UTC) #2
nweiz
Code review changes
5 years ago (2015-11-30 22:46:56 UTC) #3
nweiz
> Benchmark? Numbers? Done. https://codereview.chromium.org/1473563003/diff/1/lib/path.dart File lib/path.dart (right): https://codereview.chromium.org/1473563003/diff/1/lib/path.dart#newcode82 lib/path.dart:82: if (uri == _oldUriBase) return ...
5 years ago (2015-11-30 22:52:10 UTC) #5
nweiz
Committed patchset #2 (id:20001) manually as 414bb1e1b7928090b88386c5f15f8ddee43fe96b (presubmit successful).
5 years ago (2015-11-30 22:53:13 UTC) #7
Bob Nystrom
lgtm
5 years ago (2015-11-30 23:02:25 UTC) #8
Lasse Reichstein Nielsen
https://codereview.chromium.org/1473563003/diff/20001/lib/path.dart File lib/path.dart (right): https://codereview.chromium.org/1473563003/diff/20001/lib/path.dart#newcode89 lib/path.dart:89: _current = uri.resolve('.').toString(); That is likely to be slower ...
5 years ago (2015-12-01 06:52:58 UTC) #10
Lasse Reichstein Nielsen
https://codereview.chromium.org/1473563003/diff/20001/lib/path.dart File lib/path.dart (right): https://codereview.chromium.org/1473563003/diff/20001/lib/path.dart#newcode89 lib/path.dart:89: _current = uri.resolve('.').toString(); (That does assume that Uri.base has ...
5 years ago (2015-12-01 06:53:54 UTC) #11
nweiz
5 years ago (2015-12-01 20:50:46 UTC) #12
Message was sent while issue was closed.
https://codereview.chromium.org/1473563003/diff/20001/lib/path.dart
File lib/path.dart (right):

https://codereview.chromium.org/1473563003/diff/20001/lib/path.dart#newcode89
lib/path.dart:89: _current = uri.resolve('.').toString();
On 2015/12/01 06:53:54, Lasse Reichstein Nielsen wrote:
> (That does assume that Uri.base has no query or fragment, so maybe it's not
> general enough).

Given that this is cached now, I think it's probably not worth putting a lot of
effort into optimizing the code to compute the cached value.

Powered by Google App Engine
This is Rietveld 408576698