Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
12049058:
Adds AVX detection to CPU and records a histogram of Intel uArch Support (Closed)
|
Created:
7 years, 11 months ago by whunt Modified:
7 years, 10 months ago CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, erikwright+watch_chromium.org, jam Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdds AVX detection to CPU and records a histogram of Intel Microarchitecture Support
I added detection of AVX as well. I'm not really sure the best place to put the test. It's currently run at startup time. The patch stores and Enumerated Histogram for the supported micro-architecture.
BUG=171824
NOTRY=true
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=180223
Patch Set 1 #
Total comments: 1
Patch Set 2 : moving the check to chrome_browser_main.cc #
Total comments: 4
Patch Set 3 : addressed refactoring and renaming recommendataions #
Total comments: 6
Patch Set 4 : addressing feedback #
Messages
Total messages: 23 (0 generated)
Can you guys add some people from base and/or wherever else is most appropriate to review this?
Hey Al, is this a reasonable-ish place to collect this histogram?
I don't think this is the right place to do it because it will fail to collect SSE stats for machines that are blacklisted which could skew the results. Also there's no need to do it on every GPU process launch.
Seems useful to split up into the base/ edit and the histogram edit
Is there a "main" routine or other entry point that I could drop it into? That would certainly catch everyone but skew results towards people who restart chrome often. There's some debate as to what exactly is the best place to gather this data. From what I understand we can always dremil for users so it should be easy to get the number of unique machines with each uISA regardless of where we actually gather the data but perhaps we want to measure a result skewed to users who use chrome more. E.g. we can put it somewhere that gets called each time someone follows a link or in something that executes on a reliable heartbeat so we get a result of uISA support * amount of time it's used. Given that we're not pegged at 60hz when rendering, I don't think it's a good idea to put it in a render call is a great idea. Does anyone have any ideas or opinions? -Warren On Wed, Jan 23, 2013 at 5:36 PM, <apatrick@chromium.org> wrote: > I don't think this is the right place to do it because it will fail to > collect > SSE stats for machines that are blacklisted which could skew the results. > Also > there's no need to do it on every GPU process launch. > > https://codereview.chromium.**org/12049058/<https://codereview.chromium.org/1... >
How about chrome_browser_main.cc?
On 2013/01/24 02:07:53, apatrick_chromium wrote: > How about chrome_browser_main.cc? Does this seem good to everyone?
https://codereview.chromium.org/12049058/diff/1/content/browser/gpu/gpu_proce... File content/browser/gpu/gpu_process_host.cc (right): https://codereview.chromium.org/12049058/diff/1/content/browser/gpu/gpu_proce... content/browser/gpu/gpu_process_host.cc:278: arch = 1; nit: use an enum and comment that it must not be renumbered and that it has to be kept in sync with histograms.xml. https://codereview.chromium.org/12049058/diff/7001/chrome/browser/chrome_brow... File chrome/browser/chrome_browser_main.cc (right): https://codereview.chromium.org/12049058/diff/7001/chrome/browser/chrome_brow... chrome/browser/chrome_browser_main.cc:975: #if defined(ARCH_CPU_X86_FAMILY) Consider extracting this code to a function defined in another file and calling it from here. https://codereview.chromium.org/12049058/diff/7001/chrome/browser/chrome_brow... chrome/browser/chrome_browser_main.cc:978: if (cpu.has_sse()) arch = 1; style nit: Drop body to its own line. Also can you use an enum, make the last field be the number of fields (currently 8) and comment the enum with a warning that it must not be reordered and that it must be kept in sync with histograms.xml? https://codereview.chromium.org/12049058/diff/7001/chrome/browser/chrome_brow... chrome/browser/chrome_browser_main.cc:985: UMA_HISTOGRAM_ENUMERATION("Platform.IntelMaxuArchSupport", arch, 8); A search for "Intel Maxu Arch" yields nothing. What is "Maxu"? Is it "Max"?
https://codereview.chromium.org/12049058/diff/7001/chrome/browser/chrome_brow... File chrome/browser/chrome_browser_main.cc (right): https://codereview.chromium.org/12049058/diff/7001/chrome/browser/chrome_brow... chrome/browser/chrome_browser_main.cc:985: UMA_HISTOGRAM_ENUMERATION("Platform.IntelMaxuArchSupport", arch, 8); On 2013/01/25 19:06:30, apatrick_chromium wrote: > A search for "Intel Maxu Arch" yields nothing. What is "Maxu"? Is it "Max"? uArch is a very common (at least at Intel...) abbreviation for micro architecture (following the m, u <- greek mu, p convention for mili, micro, pico) It doen't really work if you capitalize it. I'll make Micro more explicit.
microarchitecture
erm, can we just use the word microarchitecture instead of using abbreviations? We run on arm too :)
Yeah, I changed it. On Wed, Jan 30, 2013 at 6:01 PM, <nduca@chromium.org> wrote: > erm, can we just use the word microarchitecture instead of using > abbreviations? > We run on arm too :) > > https://codereview.chromium.**org/12049058/<https://codereview.chromium.org/1... >
LGTM with some style nits. +brettw for chrome/ and base/ owners. https://codereview.chromium.org/12049058/diff/13001/base/cpu.h File base/cpu.h (right): https://codereview.chromium.org/12049058/diff/13001/base/cpu.h#newcode21 base/cpu.h:21: Pentium, Pentium -> PENTIUM https://codereview.chromium.org/12049058/diff/13001/base/cpu.h#newcode29 base/cpu.h:29: MaxIntelMicroArchitecture MaxIntelMicroArchitecture -> MAX_INTEL_MICRO_ARCHITECTURE https://codereview.chromium.org/12049058/diff/13001/base/cpu.h#newcode48 base/cpu.h:48: IntelMicroArchitecture getIntelMicroArchitecture() const; getIntelMicroArchitecture -> GetIntelMicroArchitecture
You put it in browser process creation rather than gpu process creation like your commit message says. Was this a mistake? https://codereview.chromium.org/12049058/diff/13001/base/cpu.cc File base/cpu.cc (right): https://codereview.chromium.org/12049058/diff/13001/base/cpu.cc#newcode142 base/cpu.cc:142: IntelMicroArchitecture arch = Pentium; This seems weird. It looks like if the processor supports all of these things, you'll execute every one overwriting the result. I guess it's correct but it looks like a bug. I think it would be clearer to invert and do if (has_avx()) return AVX; if (has_sse42()) return SSE42; ... https://codereview.chromium.org/12049058/diff/13001/chrome/browser/chrome_bro... File chrome/browser/chrome_browser_main.cc (right): https://codereview.chromium.org/12049058/diff/13001/chrome/browser/chrome_bro... chrome/browser/chrome_browser_main.cc:1005: #if defined(ARCH_CPU_X86_FAMILY) If this is the best place you can find, at least put in a separate function at the top of this file and call it here to keep this big function simpler. https://codereview.chromium.org/12049058/diff/13001/chrome/browser/chrome_bro... chrome/browser/chrome_browser_main.cc:1010: #endif // defined(ARCH_CPU_X86_FAMILY) need two spaces before //
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/whunt@chromium.org/12049058/24001
Presubmit check for 12049058-24001 failed and returned exit status 1.
Running presubmit commit checks ...
** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
chrome
base
Presubmit checks took 1.1s to calculate.
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/whunt@chromium.org/12049058/24001
Retried try job too often on win_rel for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
On 2013/02/01 22:38:42, I haz the power (commit-bot) wrote: > Retried try job too often on win_rel for step(s) browser_tests > http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu... I don't think these issues are caused by this patch. Is there a way to push it though if that's the right thing to do?
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/whunt@chromium.org/12049058/24001
Message was sent while issue was closed.
Change committed as 180223 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
