Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1046363002:
Enforce lowercase switches when calling CommandLine::HasSwitch(const char*). (Closed)
|
Created:
5 years, 8 months ago by jackhou1 Modified:
5 years, 8 months ago CC:
chromium-reviews, erikwright+watch_chromium.org, chrome-apps-syd-reviews_chromium.org, Nico Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionEnforce lowercase switches when calling CommandLine::HasSwitch.
At the moment, all compile-time switches are lowercase. By enforcing
this, we can skip converting it to lowercase on Windows, which saves
one string allocation per call.
On a profile with 2 extensions, HasSwitch is called ~12k times during
startup. In an ideal situation (no paging/cache pressure), the
string allocation under Windows takes ~137ns on an Xeon E5-2690 @
2.9Ghz. So this should shave off at least 1.6ms off a typical startup
with this hardware. For context,
Startup.BrowserMessageLoopStartTimeFromMainEntry is typically
280-300ms on the same hardware, so we should get a ~0.5% improvement.
BUG=472383
Committed: https://crrev.com/f58961749a980032241fe6c3fc829ac2e6652030
Cr-Commit-Position: refs/heads/master@{#325576}
Committed: https://crrev.com/b20cbb495422e5f76f3064d73d3bb9c51113c45d
Cr-Commit-Position: refs/heads/master@{#326219}
Patch Set 1 #Patch Set 2 : Remove EXPECT_DEBUG_DEATH #
Total comments: 10
Patch Set 3 : Address comments. #Patch Set 4 : Enforce lowercase for both versions of HasSwitch. #Patch Set 5 : Fix test on Linux. #
Messages
Total messages: 28 (7 generated)
jackhou@chromium.org changed reviewers: + tapted@chromium.org
tapted, could you take a look?
The CQ bit was checked by jackhou@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1046363002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
https://codereview.chromium.org/1046363002/diff/20001/base/command_line.cc File base/command_line.cc (right): https://codereview.chromium.org/1046363002/diff/20001/base/command_line.cc#ne... base/command_line.cc:273: bool CommandLine::HasSwitch(const std::string& switch_string) const { How many times is the std::string version called now? Maybe it's not worth changing this fn. https://codereview.chromium.org/1046363002/diff/20001/base/command_line.cc#ne... base/command_line.cc:284: DCHECK(!ContainsUpperASCII(string_constant)); I think we can avoid the extra function with something like DCHECK_EQ(StringToLowerASCII(string_constant), string_constant); https://codereview.chromium.org/1046363002/diff/20001/base/command_line.cc#ne... base/command_line.cc:285: return switches_.find(std::string(string_constant)) != switches_.end(); is the std::string(..) still needed? https://codereview.chromium.org/1046363002/diff/20001/base/command_line.cc#ne... base/command_line.cc:309: #if defined(OS_WIN) I think we should defer this until there's a char[] overload of this function and we have baseline on that https://codereview.chromium.org/1046363002/diff/20001/base/command_line_unitt... File base/command_line_unittest.cc (right): https://codereview.chromium.org/1046363002/diff/20001/base/command_line_unitt... base/command_line_unittest.cc:63: EXPECT_TRUE(cl.HasSwitch(std::string("bAr"))); needs a comment
also, like ben said, the CL description should have some numbers in it. *ideally* some existing chrome perf measurement goes down perceptibly. But, realistically, a link to the stuff you've measured. E.g. On a profile with X extensions, HasSwitch is called Y times during startup. In an ideal situation (no swapping/cache pressure), the string allocation [under Windows] takes [150 microseconds] [with this hardware]. So this should shave off at least [Z milliseconds] off a typical startup [with this hardware].
I tried running a perf benchmark, but the bot timed out, second attempt here: https://codereview.chromium.org/1056443004/ https://codereview.chromium.org/1046363002/diff/20001/base/command_line.cc File base/command_line.cc (right): https://codereview.chromium.org/1046363002/diff/20001/base/command_line.cc#ne... base/command_line.cc:273: bool CommandLine::HasSwitch(const std::string& switch_string) const { On 2015/04/02 01:46:14, tapted wrote: > How many times is the std::string version called now? Maybe it's not worth > changing this fn. Agreed. 1% of calls. https://codereview.chromium.org/1046363002/diff/20001/base/command_line.cc#ne... base/command_line.cc:284: DCHECK(!ContainsUpperASCII(string_constant)); On 2015/04/02 01:46:14, tapted wrote: > I think we can avoid the extra function with something like > > DCHECK_EQ(StringToLowerASCII(string_constant), string_constant); > Done. https://codereview.chromium.org/1046363002/diff/20001/base/command_line.cc#ne... base/command_line.cc:285: return switches_.find(std::string(string_constant)) != switches_.end(); On 2015/04/02 01:46:15, tapted wrote: > is the std::string(..) still needed? Nope, but the one in the DEBUG_EQ is. https://codereview.chromium.org/1046363002/diff/20001/base/command_line.cc#ne... base/command_line.cc:309: #if defined(OS_WIN) On 2015/04/02 01:46:14, tapted wrote: > I think we should defer this until there's a char[] overload of this function > and we have baseline on that This method is about 9% of lookups during startup, but yeah, there's probably more to gain by adding a char[] overload. https://codereview.chromium.org/1046363002/diff/20001/base/command_line_unitt... File base/command_line_unittest.cc (right): https://codereview.chromium.org/1046363002/diff/20001/base/command_line_unitt... base/command_line_unittest.cc:63: EXPECT_TRUE(cl.HasSwitch(std::string("bAr"))); On 2015/04/02 01:46:15, tapted wrote: > needs a comment Done.
code lgtm CL description: "paging" is probably slightly more accurate than "swapping" -- swapping comes later :) (when stuff is paged out rather than in) Then it would be good to have a baseline to compare the 1.6ms to. I think warm-start perf measurements are typically ~200ms? Maybe you can measure on the same hardware, so you can say 1.6 ~= 1%, which is a worthwhile gain IMO
jackhou@chromium.org changed reviewers: + brettw@chromium.org
brettw, could you take a look?
Not LGTM, this doesn't seem fully baked. But the number of calls is really shocking and we should definitely be looking at this. I actually think we should be doing more. I don't think it makes sense to have two versions of HasSwitch, one that takes a char* and requires it to be lower-case, and one that takes a std::string and doesn't. These should be consistent. The comment in the header for HasSwitch is incrrect now. I also note that the char* version which most people will use still does a string allocation, and we should be able to avoid this. Given the number of calls, it seems worthwhile investing some time in this. Do you think it's possible to make CommandLine's map use StringPiece instead? This will require some careful memory management inside the CommandLine class, especially about adding switches, auditing all callers of GetSwitches to make sure they don't make a copy and then have expectations about it. But then we can have one HasSwitch version instead of two, and none of the getters will involve string allocations.
On 2015/04/02 18:39:35, brettw wrote: > Not LGTM, this doesn't seem fully baked. But the number of calls is really > shocking and we should definitely be looking at this. I actually think we should > be doing more. > > I don't think it makes sense to have two versions of HasSwitch, one that takes a > char* and requires it to be lower-case, and one that takes a std::string and > doesn't. These should be consistent. Agreed. In this CL I've changed HasSwitch to simply DCHECK that all switches are lowercase. I think this a good first step to avoid the allocation on Windows to convert to lowercase. > The comment in the header for HasSwitch is incrrect now. Fixed. > I also note that the char* version which most people will use still does a > string allocation, and we should be able to avoid this. Given the number of > calls, it seems worthwhile investing some time in this. I've looked into ways to avoid this. One is to make CommandLine read-only, at which point we can sort a vector of switches and lookup using std::lower_bound (http://crrev.com/1046073002/). But I think your idea below is probably better. BTW about 91% of calls to HasSwitch pass a const char* (more details on bug: http://crbug.com/472383). > Do you think it's possible to make CommandLine's map use StringPiece instead? > This will require some careful memory management inside the CommandLine class, > especially about adding switches, auditing all callers of GetSwitches to make > sure they don't make a copy and then have expectations about it. > > But then we can have one HasSwitch version instead of two, and none of the > getters will involve string allocations. I've implemented this here: http://crrev.com/1063933002. I decided to retain the current SwitchMap and add a parallel map of StringPieces. This means we don't need to update callers of GetSwitches. Since CommandLines typically have 3-4 switches, and renderer processes have ~16, I think the memory and insertion time costs are negligible. I posted some very basic benchmarks of the various options on the bug. The main savings are from avoiding string allocation.
brettw, PTAL
Sorry, I haven't gotten to this yet, will look today.
lgtm
The CQ bit was checked by jackhou@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tapted@chromium.org Link to the patchset: https://codereview.chromium.org/1046363002/#ps80001 (title: "Fix test on Linux.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1046363002/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/f58961749a980032241fe6c3fc829ac2e6652030 Cr-Commit-Position: refs/heads/master@{#325576}
Message was sent while issue was closed.
A revert of this CL (patchset #5 id:80001) has been created in https://codereview.chromium.org/1091993002/ by yoichio@chromium.org. The reason for reverting is: This causes test fails on Android: https://build.chromium.org/p/chromium.linux/builders/Android%20Tests%20%28dbg... The test uses upper case with HasSwitch(): https://code.google.com/p/chromium/codesearch#chromium/src/content/public/and....
On 2015/04/17 08:27:21, yoichio wrote: > A revert of this CL (patchset #5 id:80001) has been created in > https://codereview.chromium.org/1091993002/ by mailto:yoichio@chromium.org. > > The reason for reverting is: This causes test fails on Android: > https://build.chromium.org/p/chromium.linux/builders/Android%20Tests%20%28dbg... > > The test uses upper case with HasSwitch(): > https://code.google.com/p/chromium/codesearch#chromium/src/content/public/and.... Tests updated here: https://codereview.chromium.org/1088313006/ Re-committing...
The CQ bit was checked by jackhou@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1046363002/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/b20cbb495422e5f76f3064d73d3bb9c51113c45d Cr-Commit-Position: refs/heads/master@{#326219} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
