Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2500473002:
[base::FilePath] Optimize FilePath::ReferencesParent (Closed)
Description[base::FilePath] Optimize FilePath::ReferencesParent
ReferencesParent is expensive but in the majority of cases can early return if
no '..' is present in the string, significantly reducing the average cost.
When loading 1,000 tiny images (35 bytes each), the SimpleCache worker threads
spend 10.7% of their time in ReferencesParent according to linux perf (during
File::Initialize). After this change, they spend .4% of their time in ReferencesParent.
BUG=664174
Committed: https://crrev.com/f95b1d92dfe18b6350cdc0f23e34ddabb07d041d
Cr-Commit-Position: refs/heads/master@{#431632}
Patch Set 1 #
Total comments: 3
Depends on Patchset: Messages
Total messages: 17 (9 generated)
The CQ bit was checked by jkarlin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== [base::FilePath] Optimize FilePath::ReferencesParent ReferencesParent is expensive but in the majority of cases can early return if no '..' is present in the string, significantly reducing the average cost. When loading 1,000 tiny images (35 bytes each), the SimpleCache worker threads spend 10.7% of their time in ReferencesParent according to linux perf. After this change, they spend .4% of their time in ReferencesParent. BUG=664174 ========== to ========== [base::FilePath] Optimize FilePath::ReferencesParent ReferencesParent is expensive but in the majority of cases can early return if no '..' is present in the string, significantly reducing the average cost. When loading 1,000 tiny images (35 bytes each), the SimpleCache worker threads spend 10.7% of their time in ReferencesParent according to linux perf (during File::Initialize). After this change, they spend .4% of their time in ReferencesParent. BUG=664174 ==========
jkarlin@chromium.org changed reviewers: + danakj@chromium.org
danakj@ PTAL, thanks! gavinp@ FYI for SimpleCache optimization
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2500473002/diff/1/base/files/file_path.cc File base/files/file_path.cc (right): https://codereview.chromium.org/2500473002/diff/1/base/files/file_path.cc#new... base/files/file_path.cc:569: std::vector<StringType> components; This is great! Question tho: We should probably use StringPiece instead StringType here too, so we don't have to do all this mallocing and copying to do GetComponents. Would that give the same wins in your case but not need an extra walk over the string?
https://codereview.chromium.org/2500473002/diff/1/base/files/file_path.cc File base/files/file_path.cc (right): https://codereview.chromium.org/2500473002/diff/1/base/files/file_path.cc#new... base/files/file_path.cc:569: std::vector<StringType> components; On 2016/11/11 19:17:40, danakj wrote: > This is great! > > Question tho: We should probably use StringPiece instead StringType here too, so > we don't have to do all this mallocing and copying to do GetComponents. Would > that give the same wins in your case but not need an extra walk over the string? Are you suggesting that GetComponents should take a vector<StringPieceType> and fill it accordingly? That would be better, but it's non-trivial as GetComponents depends on BaseName() and DirName() which both return FilePath types. We could also rewrite this function to just walk down the path scanning for '..'s and ignoring the whitespace on either side. That would result in a fairly fast function, but I bet you that average case this function is still faster. I don't think we're going to see '..' in paths very often.
https://codereview.chromium.org/2500473002/diff/1/base/files/file_path.cc File base/files/file_path.cc (right): https://codereview.chromium.org/2500473002/diff/1/base/files/file_path.cc#new... base/files/file_path.cc:569: std::vector<StringType> components; On 2016/11/11 19:38:24, jkarlin wrote: > On 2016/11/11 19:17:40, danakj wrote: > > This is great! > > > > Question tho: We should probably use StringPiece instead StringType here too, > so > > we don't have to do all this mallocing and copying to do GetComponents. Would > > that give the same wins in your case but not need an extra walk over the > string? > > Are you suggesting that GetComponents should take a vector<StringPieceType> and > fill it accordingly? That would be better, but it's non-trivial as GetComponents > depends on BaseName() and DirName() which both return FilePath types. Ah ok, it's not trivial at all np. This LGTM! > We could also rewrite this function to just walk down the path scanning for > '..'s and ignoring the whitespace on either side. That would result in a fairly > fast function, but I bet you that average case this function is still faster. I > don't think we're going to see '..' in paths very often. For most users I think you're right. Maybe someone will have a .. in their XDG_CONFIG_PATH or something and then they'll hit it all the time. But it seems corner-casey.
Thanks!
The CQ bit was checked by jkarlin@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== [base::FilePath] Optimize FilePath::ReferencesParent ReferencesParent is expensive but in the majority of cases can early return if no '..' is present in the string, significantly reducing the average cost. When loading 1,000 tiny images (35 bytes each), the SimpleCache worker threads spend 10.7% of their time in ReferencesParent according to linux perf (during File::Initialize). After this change, they spend .4% of their time in ReferencesParent. BUG=664174 ========== to ========== [base::FilePath] Optimize FilePath::ReferencesParent ReferencesParent is expensive but in the majority of cases can early return if no '..' is present in the string, significantly reducing the average cost. When loading 1,000 tiny images (35 bytes each), the SimpleCache worker threads spend 10.7% of their time in ReferencesParent according to linux perf (during File::Initialize). After this change, they spend .4% of their time in ReferencesParent. BUG=664174 ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1)
Message was sent while issue was closed.
Description was changed from ========== [base::FilePath] Optimize FilePath::ReferencesParent ReferencesParent is expensive but in the majority of cases can early return if no '..' is present in the string, significantly reducing the average cost. When loading 1,000 tiny images (35 bytes each), the SimpleCache worker threads spend 10.7% of their time in ReferencesParent according to linux perf (during File::Initialize). After this change, they spend .4% of their time in ReferencesParent. BUG=664174 ========== to ========== [base::FilePath] Optimize FilePath::ReferencesParent ReferencesParent is expensive but in the majority of cases can early return if no '..' is present in the string, significantly reducing the average cost. When loading 1,000 tiny images (35 bytes each), the SimpleCache worker threads spend 10.7% of their time in ReferencesParent according to linux perf (during File::Initialize). After this change, they spend .4% of their time in ReferencesParent. BUG=664174 Committed: https://crrev.com/f95b1d92dfe18b6350cdc0f23e34ddabb07d041d Cr-Commit-Position: refs/heads/master@{#431632} ==========
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/f95b1d92dfe18b6350cdc0f23e34ddabb07d041d Cr-Commit-Position: refs/heads/master@{#431632} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
