Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
61983003:
Add a switch to emit browser histograms. (Closed)
|
Created:
7 years, 1 month ago by grt (UTC plus 2) Modified:
7 years, 1 month ago CC:
chromium-reviews, erikwright+watch_chromium.org, Ilya Sherman, jar (doing other things), asvitkine+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdd a switch to emit browser histograms.
BUG=None
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=235191
Patch Set 1 #Patch Set 2 : Use histogram to JSON code from https://codereview.chromium.org/55363003/. #
Total comments: 16
Patch Set 3 : review comments #
Total comments: 4
Patch Set 4 : more review comments #
Total comments: 16
Patch Set 5 : final nits addressed #Patch Set 6 : fix comments #
Messages
Total messages: 30 (0 generated)
jar: base/metrics/* pk: chrome/* thanks.
This looks similar to the current pending CL here: https://codereview.chromium.org/55363003/ (Both exporting as JSON.) You should probably just make the command-line switch use the functionality being added in that CL, if possible.
On 2013/11/06 18:37:12, Alexei Svitkine wrote: > This looks similar to the current pending CL here: > > https://codereview.chromium.org/55363003/ > > (Both exporting as JSON.) > > You should probably just make the command-line switch use the functionality > being added in that CL, if possible. It looks like I could use StatisticsRecorder::WriteJSON. Thanks for the pointer. Do you have any idea when that is expected to land?
chrome/common LGTM. I'll hold off on chrome/browser since it sounds like that might change based on the other CL.
On 2013/11/06 19:14:05, grt wrote: > On 2013/11/06 18:37:12, Alexei Svitkine wrote: > > This looks similar to the current pending CL here: > > > > https://codereview.chromium.org/55363003/ > > > > (Both exporting as JSON.) > > > > You should probably just make the command-line switch use the functionality > > being added in that CL, if possible. > > It looks like I could use StatisticsRecorder::WriteJSON. Thanks for the pointer. > Do you have any idea when that is expected to land? I think pretty soon, it was actually aiming for M32 but looks like they didn't get it in (not sure if they'll try to merge it back).
base/metrics LGTM
Sending out for re-review. Everything but chrome/common has changed. The overall result is the same, except that I've switched to using the histogram-to-string code being introduced in https://codereview.chromium.org/55363003/ (hat tip to Alexei for pointing that out). jar: base/metrics/* pk: chrome/* Thanks.
Tiny item I missed the first time... https://codereview.chromium.org/61983003/diff/130001/base/metrics/statistics_... File base/metrics/statistics_recorder.cc (right): https://codereview.chromium.org/61983003/diff/130001/base/metrics/statistics_... base/metrics/statistics_recorder.cc:192: if (snapshot.size()) if the list of histograms is empty, then you might not ever run line 189... so there might not be a trailing comma inside the square brackets. You could test for the trailing comma (before trying to delete it), but IMO, it is better to not write any (extra) comma in the first place (backtracking is always dangerous). To do this, I'd suggest a pattern like: bool first_time = true; for(...) { if (first_time) first_time = false; else output->append(","); Other folks like using: std::string separator = ""; before the loop and then updating it once you pass through the loop: separator = ","; YMMV.
https://codereview.chromium.org/61983003/diff/130001/chrome/browser/ui/startu... File chrome/browser/ui/startup/startup_browser_creator.cc (right): https://codereview.chromium.org/61983003/diff/130001/chrome/browser/ui/startu... chrome/browser/ui/startup/startup_browser_creator.cc:233: // To be called only on the blocking pool. Nit: I'd put a comment like this above the function as "This function should only be called on the blocking pool." https://codereview.chromium.org/61983003/diff/130001/chrome/browser/ui/startu... chrome/browser/ui/startup/startup_browser_creator.cc:236: if (base::PathExists(output_file)) Maybe we should overwrite or append to the file instead of doing nothing? https://codereview.chromium.org/61983003/diff/130001/chrome/browser/ui/startu... chrome/browser/ui/startup/startup_browser_creator.cc:240: base::StatisticsRecorder::WriteJSON(std::string(), &output_string); Could you perhaps also change the WriteJSON() method to return the string by value instead of by outparam? This should be just as fast due to RVO. (See my email to chromium-dev tonight about this issue.) It should also allow making HistogramMessageFilter's code a little simpler as an added bonus. https://codereview.chromium.org/61983003/diff/130001/chrome/browser/ui/startu... chrome/browser/ui/startup/startup_browser_creator.cc:241: if (output_string.size() <= INT_MAX) { Urgh... I wonder if we can assume histograms are significantly smaller than INT_MAX total bytes, such that this check isn't really necessary. (If it is necessary, we may want to consider changing the file_util interfaces to use 64-bit length types.) https://codereview.chromium.org/61983003/diff/130001/chrome/browser/ui/startu... chrome/browser/ui/startup/startup_browser_creator.cc:622: if (!process_startup) { I lack sufficient familiarity with this file to truly understand |process_statup| and |silent_launch|. Could you find a reviewer who's touched these before and can verify you're using them correctly?
Thanks for the comments, PTAL. +marja: would you please take a look at the use of |process_startup| and |silent_launch| in StartupBrowserCreator::ProcessCmdLineImpl to see if they're correct? The intent is that running "chrome.exe --dump-browser-histograms=/some/file/path" will rendezvous with a running browser process and cause it to emit its collected histograms to the given file. Please redirect if you know of someone better suited to check this. Thanks. https://codereview.chromium.org/61983003/diff/130001/base/metrics/statistics_... File base/metrics/statistics_recorder.cc (right): https://codereview.chromium.org/61983003/diff/130001/base/metrics/statistics_... base/metrics/statistics_recorder.cc:192: if (snapshot.size()) On 2013/11/13 01:25:46, jar wrote: > if the list of histograms is empty, then you might not ever run line 189... so > there might not be a trailing comma inside the square brackets. > > You could test for the trailing comma (before trying to delete it), but IMO, it > is better to not write any (extra) comma in the first place (backtracking is > always dangerous). To do this, I'd suggest a pattern like: > > bool first_time = true; > for(...) { > if (first_time) > first_time = false; > else > output->append(","); > > Other folks like using: > std::string separator = ""; > before the loop and then updating it once you pass through the loop: > separator = ","; > > YMMV. Done. https://codereview.chromium.org/61983003/diff/130001/chrome/browser/ui/startu... File chrome/browser/ui/startup/startup_browser_creator.cc (right): https://codereview.chromium.org/61983003/diff/130001/chrome/browser/ui/startu... chrome/browser/ui/startup/startup_browser_creator.cc:233: // To be called only on the blocking pool. On 2013/11/13 03:49:21, Peter Kasting wrote: > Nit: I'd put a comment like this above the function as "This function should > only be called on the blocking pool." Done. https://codereview.chromium.org/61983003/diff/130001/chrome/browser/ui/startu... chrome/browser/ui/startup/startup_browser_creator.cc:236: if (base::PathExists(output_file)) On 2013/11/13 03:49:21, Peter Kasting wrote: > Maybe we should overwrite or append to the file instead of doing nothing? I considered but discarded both. Overwrite introduces a possibility for accidental data loss, while a pure append would result in an invalid JSON file. I think this is sufficiently simple to leave as-is unless you feel strongly otherwise. https://codereview.chromium.org/61983003/diff/130001/chrome/browser/ui/startu... chrome/browser/ui/startup/startup_browser_creator.cc:240: base::StatisticsRecorder::WriteJSON(std::string(), &output_string); On 2013/11/13 03:49:21, Peter Kasting wrote: > Could you perhaps also change the WriteJSON() method to return the string by > value instead of by outparam? This should be just as fast due to RVO. (See my > email to chromium-dev tonight about this issue.) It should also allow making > HistogramMessageFilter's code a little simpler as an added bonus. Thanks for the suggestion. I'm going to pass on this both to keep the change small and because RVO isn't as reliable as taking the out param. https://codereview.chromium.org/61983003/diff/130001/chrome/browser/ui/startu... chrome/browser/ui/startup/startup_browser_creator.cc:241: if (output_string.size() <= INT_MAX) { On 2013/11/13 03:49:21, Peter Kasting wrote: > Urgh... I wonder if we can assume histograms are significantly smaller than > INT_MAX total bytes, such that this check isn't really necessary. I feel pretty confident that they'll be smaller than MAX_INT, but I don't like writing incorrect code. I'm checking with jschuh@ (owner of crbug.com/167187) to see if he has a cleaner suggestion. https://codereview.chromium.org/61983003/diff/130001/chrome/browser/ui/startu... chrome/browser/ui/startup/startup_browser_creator.cc:622: if (!process_startup) { On 2013/11/13 03:49:21, Peter Kasting wrote: > I lack sufficient familiarity with this file to truly understand > |process_statup| and |silent_launch|. Could you find a reviewer who's touched > these before and can verify you're using them correctly? Done.
https://codereview.chromium.org/61983003/diff/380001/chrome/browser/ui/startu... File chrome/browser/ui/startup/startup_browser_creator.cc (right): https://codereview.chromium.org/61983003/diff/380001/chrome/browser/ui/startu... chrome/browser/ui/startup/startup_browser_creator.cc:622: if (!process_startup) { Doing some code searching, it seems process_startup is also false when we're creating a new profile and launch it. In that case previously, silent_launch was not set to true, and now you're setting it to true. That doesn't seem right.
lgtm with a nit and % other people's comments https://codereview.chromium.org/61983003/diff/380001/chrome/browser/ui/startu... File chrome/browser/ui/startup/startup_browser_creator.cc (right): https://codereview.chromium.org/61983003/diff/380001/chrome/browser/ui/startu... chrome/browser/ui/startup/startup_browser_creator.cc:232: // This function should only be called in the blocking pool. Nit: Have the comment actually briefly mention what this does.
base/metrics LGTM
base/metrics LGTM
https://codereview.chromium.org/61983003/diff/130001/chrome/browser/ui/startu... File chrome/browser/ui/startup/startup_browser_creator.cc (right): https://codereview.chromium.org/61983003/diff/130001/chrome/browser/ui/startu... chrome/browser/ui/startup/startup_browser_creator.cc:240: base::StatisticsRecorder::WriteJSON(std::string(), &output_string); On 2013/11/13 15:13:00, grt wrote: > On 2013/11/13 03:49:21, Peter Kasting wrote: > > Could you perhaps also change the WriteJSON() method to return the string by > > value instead of by outparam? This should be just as fast due to RVO. (See > my > > email to chromium-dev tonight about this issue.) It should also allow making > > HistogramMessageFilter's code a little simpler as an added bonus. > > Thanks for the suggestion. I'm going to pass on this both to keep the change > small and because RVO isn't as reliable as taking the out param. Then can we do this as a followon change? It's reasonable to not do it here, but especially for strings, our style guide explicitly suggests returning by value in preference to using outparams. https://codereview.chromium.org/61983003/diff/130001/chrome/browser/ui/startu... chrome/browser/ui/startup/startup_browser_creator.cc:241: if (output_string.size() <= INT_MAX) { On 2013/11/13 15:13:00, grt wrote: > On 2013/11/13 03:49:21, Peter Kasting wrote: > > Urgh... I wonder if we can assume histograms are significantly smaller than > > INT_MAX total bytes, such that this check isn't really necessary. > > I feel pretty confident that they'll be smaller than MAX_INT, but I don't like > writing incorrect code. I'm checking with jschuh@ (owner of crbug.com/167187) to > see if he has a cleaner suggestion. Well, if your definition of correct code is that we always size-check all values before putting them into a type, I've got about 100 million other places in Chrome for you to fix. INT_MAX on the targets we build on ought to allow for 2 gigabytes of histogram data. Could we really even come within two orders of magnitude of this value? Furthermore, would we really be able to allocate a single 2-gigabyte string in memory to return this value, in addition to all the memory used elsewhere in Chrome, not least the original memory used for the histogram data itself? It seems like this is beyond "let's be technically correct" and into the way-paranoid state. Yet writing it here implies that this actually can happen, which misleads readers; and the added verbosity doesn't help anything. I really don't think we should do this.
All comments addressed. PTAL. https://codereview.chromium.org/61983003/diff/130001/chrome/browser/ui/startu... File chrome/browser/ui/startup/startup_browser_creator.cc (right): https://codereview.chromium.org/61983003/diff/130001/chrome/browser/ui/startu... chrome/browser/ui/startup/startup_browser_creator.cc:240: base::StatisticsRecorder::WriteJSON(std::string(), &output_string); On 2013/11/13 19:18:26, Peter Kasting wrote: > On 2013/11/13 15:13:00, grt wrote: > > On 2013/11/13 03:49:21, Peter Kasting wrote: > > > Could you perhaps also change the WriteJSON() method to return the string by > > > value instead of by outparam? This should be just as fast due to RVO. (See > > my > > > email to chromium-dev tonight about this issue.) It should also allow > making > > > HistogramMessageFilter's code a little simpler as an added bonus. > > > > Thanks for the suggestion. I'm going to pass on this both to keep the change > > small and because RVO isn't as reliable as taking the out param. > > Then can we do this as a followon change? It's reasonable to not do it here, > but especially for strings, our style guide explicitly suggests returning by > value in preference to using outparams. I've gone ahead and made this change for the new StatisticsRecorder method introduced here. I suspect the original author (in https://codereview.chromium.org/55363003/) had it taking the string as an out param because of its similarity to StatisticsRecorder::WriteHTMLGraph and StatisticsRecorder::WriteGraph, both of which append to an existing string. I have not changed those, but I have renamed the new method to ToJSON (with a doc comment), and have improved the doc comments for the other two Write* methods. https://codereview.chromium.org/61983003/diff/130001/chrome/browser/ui/startu... chrome/browser/ui/startup/startup_browser_creator.cc:241: if (output_string.size() <= INT_MAX) { On 2013/11/13 19:18:26, Peter Kasting wrote: > On 2013/11/13 15:13:00, grt wrote: > > On 2013/11/13 03:49:21, Peter Kasting wrote: > > > Urgh... I wonder if we can assume histograms are significantly smaller than > > > INT_MAX total bytes, such that this check isn't really necessary. > > > > I feel pretty confident that they'll be smaller than MAX_INT, but I don't like > > writing incorrect code. I'm checking with jschuh@ (owner of crbug.com/167187) > to > > see if he has a cleaner suggestion. > > Well, if your definition of correct code is that we always size-check all values > before putting them into a type, I've got about 100 million other places in > Chrome for you to fix. > > INT_MAX on the targets we build on ought to allow for 2 gigabytes of histogram > data. Could we really even come within two orders of magnitude of this value? Likely not. > Furthermore, would we really be able to allocate a single 2-gigabyte string in > memory to return this value, in addition to all the memory used elsewhere in > Chrome, not least the original memory used for the histogram data itself? If not now on an x64 system with an x64 build of Chrome, then soon, yes. > It seems like this is beyond "let's be technically correct" and into the > way-paranoid state. Yet writing it here implies that this actually can happen, > which misleads readers; and the added verbosity doesn't help anything. I really > don't think we should do this. I've removed it for now since I agree it's not likely today, certainly not with 32-bit builds. I'm still waiting with curiosity for jschuh to get back to me, though. I feel less comfortable with incorrect casts than you do. These things tend to get casually copy-n-pasted around the codebase. Having a consistent rule for proper range checking scales better than ad-hoc decisions. https://codereview.chromium.org/61983003/diff/380001/chrome/browser/ui/startu... File chrome/browser/ui/startup/startup_browser_creator.cc (right): https://codereview.chromium.org/61983003/diff/380001/chrome/browser/ui/startu... chrome/browser/ui/startup/startup_browser_creator.cc:232: // This function should only be called in the blocking pool. On 2013/11/13 19:04:32, Alexei Svitkine wrote: > Nit: Have the comment actually briefly mention what this does. Done. https://codereview.chromium.org/61983003/diff/380001/chrome/browser/ui/startu... chrome/browser/ui/startup/startup_browser_creator.cc:622: if (!process_startup) { On 2013/11/13 15:37:34, marja wrote: > Doing some code searching, it seems process_startup is also false when we're > creating a new profile and launch it. In that case previously, silent_launch was > not set to true, and now you're setting it to true. That doesn't seem right. Indeed. Nice catch. I had meant for silent_launch only to be set if the new switch was present. I've fixed the code and added a comment to this effect.
LGTM https://codereview.chromium.org/61983003/diff/490001/base/metrics/statistics_... File base/metrics/statistics_recorder.cc (right): https://codereview.chromium.org/61983003/diff/490001/base/metrics/statistics_... base/metrics/statistics_recorder.cc:173: if (query.length()) { Nit: Use !empty() https://codereview.chromium.org/61983003/diff/490001/base/metrics/statistics_... base/metrics/statistics_recorder.cc:174: output.append("\"query\":"); Nit: All these append() calls could just be += https://codereview.chromium.org/61983003/diff/490001/base/metrics/statistics_... base/metrics/statistics_recorder.cc:183: bool first_time = true; Nit: |first_histogram|? https://codereview.chromium.org/61983003/diff/490001/base/metrics/statistics_... base/metrics/statistics_recorder.cc:190: json.clear(); Nit: Instead of trying to micro-optimize with clear(), we should just declare |json| here. https://codereview.chromium.org/61983003/diff/490001/base/metrics/statistics_... File base/metrics/statistics_recorder.h (right): https://codereview.chromium.org/61983003/diff/490001/base/metrics/statistics_... base/metrics/statistics_recorder.h:58: // Returns the histograms with |query| as a substring as JSON text. Nit: Maybe should add "(as in Write[HTML]Graph above, passing an empty string will return all histograms)" https://codereview.chromium.org/61983003/diff/490001/base/metrics/statistics_... File base/metrics/statistics_recorder_unittest.cc (right): https://codereview.chromium.org/61983003/diff/490001/base/metrics/statistics_... base/metrics/statistics_recorder_unittest.cc:274: std::string query; Nit: We can just declare this string where it's first assigned to below, and pass std::string() in the next line.
chrome/browser/ui/startup/startup_browser_creator.cc LGTM code-wise (a design comment below) https://codereview.chromium.org/61983003/diff/490001/chrome/browser/ui/startu... File chrome/browser/ui/startup/startup_browser_creator.cc (right): https://codereview.chromium.org/61983003/diff/490001/chrome/browser/ui/startu... chrome/browser/ui/startup/startup_browser_creator.cc:238: if (base::PathExists(output_file)) I think this is confusing and error prone for the user point of view. I can imagine the use case for this feature is that you run a command line for dumping the data several times (at least I heavily recycle my command lines), and it's easy to make a mistake by specifying a file that is already there from the previous run, in which case the file would be left untouched, but the user might think the data was retrieved. I tried to come up with examples of programs (e.g., unix command line tools) that work this way, but couldn't think of any. All examples I can think of overwrite the output file if it already exists. But as I'm not familiar with the feature itself, it's up to you to decide if this is what you want.
Thanks for the reviews. https://codereview.chromium.org/61983003/diff/490001/base/metrics/statistics_... File base/metrics/statistics_recorder.cc (right): https://codereview.chromium.org/61983003/diff/490001/base/metrics/statistics_... base/metrics/statistics_recorder.cc:173: if (query.length()) { On 2013/11/14 07:04:06, Peter Kasting wrote: > Nit: Use !empty() Done. https://codereview.chromium.org/61983003/diff/490001/base/metrics/statistics_... base/metrics/statistics_recorder.cc:174: output.append("\"query\":"); On 2013/11/14 07:04:06, Peter Kasting wrote: > Nit: All these append() calls could just be += Done. https://codereview.chromium.org/61983003/diff/490001/base/metrics/statistics_... base/metrics/statistics_recorder.cc:183: bool first_time = true; On 2013/11/14 07:04:06, Peter Kasting wrote: > Nit: |first_histogram|? Done. https://codereview.chromium.org/61983003/diff/490001/base/metrics/statistics_... base/metrics/statistics_recorder.cc:190: json.clear(); On 2013/11/14 07:04:06, Peter Kasting wrote: > Nit: Instead of trying to micro-optimize with clear(), we should just declare > |json| here. Leaving this as-is. https://codereview.chromium.org/61983003/diff/490001/base/metrics/statistics_... File base/metrics/statistics_recorder.h (right): https://codereview.chromium.org/61983003/diff/490001/base/metrics/statistics_... base/metrics/statistics_recorder.h:58: // Returns the histograms with |query| as a substring as JSON text. On 2013/11/14 07:04:06, Peter Kasting wrote: > Nit: Maybe should add "(as in Write[HTML]Graph above, passing an empty string > will return all histograms)" Done. https://codereview.chromium.org/61983003/diff/490001/base/metrics/statistics_... File base/metrics/statistics_recorder_unittest.cc (right): https://codereview.chromium.org/61983003/diff/490001/base/metrics/statistics_... base/metrics/statistics_recorder_unittest.cc:274: std::string query; On 2013/11/14 07:04:06, Peter Kasting wrote: > Nit: We can just declare this string where it's first assigned to below, and > pass std::string() in the next line. Done. https://codereview.chromium.org/61983003/diff/490001/chrome/browser/ui/startu... File chrome/browser/ui/startup/startup_browser_creator.cc (right): https://codereview.chromium.org/61983003/diff/490001/chrome/browser/ui/startu... chrome/browser/ui/startup/startup_browser_creator.cc:238: if (base::PathExists(output_file)) On 2013/11/14 08:33:10, marja wrote: > I think this is confusing and error prone for the user point of view. I can > imagine the use case for this feature is that you run a command line for dumping > the data several times (at least I heavily recycle my command lines), and it's > easy to make a mistake by specifying a file that is already there from the > previous run, in which case the file would be left untouched, but the user might > think the data was retrieved. > > I tried to come up with examples of programs (e.g., unix command line tools) > that work this way, but couldn't think of any. All examples I can think of > overwrite the output file if it already exists. > > But as I'm not familiar with the feature itself, it's up to you to decide if > this is what you want. After more consideration, I think you're right. My main concern with the overwrite case was an accidental fat-fingering leading to overwriting the wrong file, but given the purpose of this switch and the burden it puts on callers, I think overwrite is good. Thanks.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/grt@chromium.org/61983003/640001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/grt@chromium.org/61983003/830001
Message was sent while issue was closed.
Change committed as 235191
Message was sent while issue was closed.
https://codereview.chromium.org/61983003/diff/490001/base/metrics/statistics_... File base/metrics/statistics_recorder.cc (right): https://codereview.chromium.org/61983003/diff/490001/base/metrics/statistics_... base/metrics/statistics_recorder.cc:190: json.clear(); On 2013/11/14 14:44:12, grt wrote: > On 2013/11/14 07:04:06, Peter Kasting wrote: > > Nit: Instead of trying to micro-optimize with clear(), we should just declare > > |json| here. > > Leaving this as-is. OK... is there a big advantage to doing things this way? This extends the apparent lifetime of the temp, which is not only more confusing to read, it hurts compiler attempts to optimize away the underlying variable.
Message was sent while issue was closed.
https://codereview.chromium.org/61983003/diff/490001/base/metrics/statistics_... File base/metrics/statistics_recorder.cc (right): https://codereview.chromium.org/61983003/diff/490001/base/metrics/statistics_... base/metrics/statistics_recorder.cc:190: json.clear(); On 2013/11/14 21:26:16, Peter Kasting wrote: > On 2013/11/14 14:44:12, grt wrote: > > On 2013/11/14 07:04:06, Peter Kasting wrote: > > > Nit: Instead of trying to micro-optimize with clear(), we should just > declare > > > |json| here. > > > > Leaving this as-is. > > OK... is there a big advantage to doing things this way? This extends the > apparent lifetime of the temp, which is not only more confusing to read, it > hurts compiler attempts to optimize away the underlying variable. I believe that reuse of the underlying buffer will reduce the number of heap operations. Is the compiler smart enough to know the underlying implementation of std::string and know that it doesn't need to call its dtor/ctor on each iteration?
Message was sent while issue was closed.
On 2013/11/14 21:52:26, grt wrote: > On 2013/11/14 21:26:16, Peter Kasting wrote: > > OK... is there a big advantage to doing things this way? This extends the > > apparent lifetime of the temp, which is not only more confusing to read, it > > hurts compiler attempts to optimize away the underlying variable. > > I believe that reuse of the underlying buffer will reduce the number of heap > operations. Is the compiler smart enough to know the underlying implementation > of std::string and know that it doesn't need to call its dtor/ctor on each > iteration? I believe you're correct that this would reduce the number of heap operations, but why does that matter? This isn't performance-critical code, and we're going to be copying this string data straight into another buffer anyway, meaning we're still going to have to potentially enlarge that buffer, then do the memcpy; so I'm not sure this would dramatically improve the performance here to begin with.
Message was sent while issue was closed.
On 2013/11/14 21:59:21, Peter Kasting wrote: > On 2013/11/14 21:52:26, grt wrote: > > On 2013/11/14 21:26:16, Peter Kasting wrote: > > > OK... is there a big advantage to doing things this way? This extends the > > > apparent lifetime of the temp, which is not only more confusing to read, it > > > hurts compiler attempts to optimize away the underlying variable. > > > > I believe that reuse of the underlying buffer will reduce the number of heap > > operations. Is the compiler smart enough to know the underlying implementation > > of std::string and know that it doesn't need to call its dtor/ctor on each > > iteration? > > I believe you're correct that this would reduce the number of heap operations, > but why does that matter? This isn't performance-critical code, and we're going > to be copying this string data straight into another buffer anyway, meaning > we're still going to have to potentially enlarge that buffer, then do the > memcpy; so I'm not sure this would dramatically improve the performance here to > begin with. I believe that simple memory management techniques such as this can have an impact on heap fragmentation and therefore performance in a large codebase such as Chrome.
Message was sent while issue was closed.
On 2013/11/15 17:32:32, grt wrote: > On 2013/11/14 21:59:21, Peter Kasting wrote: > > On 2013/11/14 21:52:26, grt wrote: > > > On 2013/11/14 21:26:16, Peter Kasting wrote: > > > > OK... is there a big advantage to doing things this way? This extends the > > > > apparent lifetime of the temp, which is not only more confusing to read, > it > > > > hurts compiler attempts to optimize away the underlying variable. > > > > > > I believe that reuse of the underlying buffer will reduce the number of heap > > > operations. Is the compiler smart enough to know the underlying > implementation > > > of std::string and know that it doesn't need to call its dtor/ctor on each > > > iteration? > > > > I believe you're correct that this would reduce the number of heap operations, > > but why does that matter? This isn't performance-critical code, and we're > going > > to be copying this string data straight into another buffer anyway, meaning > > we're still going to have to potentially enlarge that buffer, then do the > > memcpy; so I'm not sure this would dramatically improve the performance here > to > > begin with. > > I believe that simple memory management techniques such as this can have an > impact on heap fragmentation and therefore performance in a large codebase such > as Chrome. Where I'm trying to go with this is, stop speaking in general terms (in which case all optimizations would always be good ideas) and speak specifically to how this API is used. This isn't a perf-critical API. It's generally not called at all, and when it is called, perf doesn't matter. But in terms of _reading_ the code, the usage is weird. We violate the Google style guide by not declaring a variable in the most local possible scope -- which leads the reader to believe that the value of this string is needed on entry to and/or exit from the loop, which is false. And we end up with a construction that is not used elsewhere in Chrome that I can think of off the top of my head. This implies to the reader that we have a really great reason for needing to worry a lot about performance right here, enough to do something unusual like this. But that's false too. The clarity of the code's intent is just so much more important than performance here. Perhaps it seems like a small thing but this REALLY stuck out when I was reading the code and was very confusing. Were I encountering it as a maintainer later, without the context of this change, I'd definitely read the code in a different way.
Message was sent while issue was closed.
On 2013/11/15 22:28:24, Peter Kasting wrote: > On 2013/11/15 17:32:32, grt wrote: > > On 2013/11/14 21:59:21, Peter Kasting wrote: > > > On 2013/11/14 21:52:26, grt wrote: > > > > On 2013/11/14 21:26:16, Peter Kasting wrote: > > > > > OK... is there a big advantage to doing things this way? This extends > the > > > > > apparent lifetime of the temp, which is not only more confusing to read, > > it > > > > > hurts compiler attempts to optimize away the underlying variable. > > > > > > > > I believe that reuse of the underlying buffer will reduce the number of > heap > > > > operations. Is the compiler smart enough to know the underlying > > implementation > > > > of std::string and know that it doesn't need to call its dtor/ctor on each > > > > iteration? > > > > > > I believe you're correct that this would reduce the number of heap > operations, > > > but why does that matter? This isn't performance-critical code, and we're > > going > > > to be copying this string data straight into another buffer anyway, meaning > > > we're still going to have to potentially enlarge that buffer, then do the > > > memcpy; so I'm not sure this would dramatically improve the performance here > > to > > > begin with. > > > > I believe that simple memory management techniques such as this can have an > > impact on heap fragmentation and therefore performance in a large codebase > such > > as Chrome. > > Where I'm trying to go with this is, stop speaking in general terms (in which > case all optimizations would always be good ideas) and speak specifically to how > this API is used. This isn't a perf-critical API. It's generally not called at > all, and when it is called, perf doesn't matter. But in terms of _reading_ the > code, the usage is weird. We violate the Google style guide by not declaring a > variable in the most local possible scope Perhaps I'm interpreting the style guide differently, but I don't think this violates it; see http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Local_.... It's not my intention to contribute code to Chromium that violates the style guide. I do understand the importance of code health and team maintenance. > -- which leads the reader to believe > that the value of this string is needed on entry to and/or exit from the loop, > which is false. And we end up with a construction that is not used elsewhere in > Chrome that I can think of off the top of my head. This implies to the reader > that we have a really great reason for needing to worry a lot about performance > right here, enough to do something unusual like this. But that's false too. > > The clarity of the code's intent is just so much more important than performance > here. Perhaps it seems like a small thing but this REALLY stuck out when I was > reading the code and was very confusing. The impasse we have right now is that it really sticks out for me when I see it in the loop. Since the style guide doesn't tell me to do it that way that is unnatural for me, I put the variable outside of the loop. I'll change my ways if there's consensus on chromium-dev that the style guide is mis-guided in this case and that the majority of contributors share your view. > Were I encountering it as a maintainer > later, without the context of this change, I'd definitely read the code in a > different way.
Message was sent while issue was closed.
This will be my last message on this topic. On 2013/11/18 16:49:59, grt wrote: > I'll change my ways if > there's consensus on chromium-dev that the style guide is mis-guided in this > case and that the majority of contributors share your view. To summarize the thread for posterity: * One "lean slightly towards the [inside-loop] version" followed by "both fine if they perform well enough" * One "variable belongs in the loop unless this is in a critical performance path" * One "[inside the loop] most closely embodies the ethos of the style guide...[outside the loop] should require showing a performance impact or at least a strong argument of the impact" * One "outside the loop [due to Codelab about this]" followed by a retort quoting the same codelab as ending with "In general, Google style prefers clarity and simplicity, so consider only using this trick where it matters" To me this sounds like reasonable support for the idea that the inside-loop form is more consistent with Google's intended style. I leave it to you to decide whether you agree.
Message was sent while issue was closed.
On 2013/11/19 22:35:02, Peter Kasting wrote: > This will be my last message on this topic. > > On 2013/11/18 16:49:59, grt wrote: > > I'll change my ways if > > there's consensus on chromium-dev that the style guide is mis-guided in this > > case and that the majority of contributors share your view. > > To summarize the thread for posterity: > * One "lean slightly towards the [inside-loop] version" followed by "both fine > if they perform well enough" > * One "variable belongs in the loop unless this is in a critical performance > path" > * One "[inside the loop] most closely embodies the ethos of the style > guide...[outside the loop] should require showing a performance impact or at > least a strong argument of the impact" > * One "outside the loop [due to Codelab about this]" followed by a retort > quoting the same codelab as ending with "In general, Google style prefers > clarity and simplicity, so consider only using this trick where it matters" > > To me this sounds like reasonable support for the idea that the inside-loop form > is more consistent with Google's intended style. I leave it to you to decide > whether you agree. Thanks for bringing it up on chromium-dev. I've landed a change to move this string into the loop. Cheers, Greg |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
