Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
276683005:
Telemetry: Adds a Windows PGO profiler. (Closed)
|
Created:
6 years, 7 months ago by Sébastien Marchand Modified:
6 years, 7 months ago CC:
chromium-reviews, telemetry+watch_chromium.org, scottmg Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionAdds a Windows PGO profiler.
This profiler will take care of gathering the profiling data used by the PGO builders on Windows.
Because of the aggressive way Chrome shutdown its processes we need to manually call pgosweep.exe (http://msdn.microsoft.com/en-us/library/9hwkw6e8.aspx) to dump the profiling data into a PGC file.
BUG=309849
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=272293
Patch Set 1 #
Total comments: 12
Patch Set 2 : Address Scott's comments. #
Total comments: 4
Messages
Total messages: 11 (0 generated)
PTAL.
+scottmg@ (in cc) FYI.
non-owner lg https://codereview.chromium.org/276683005/diff/1/tools/telemetry/telemetry/co... File tools/telemetry/telemetry/core/platform/profiler/win_pgo_profiler.py (right): https://codereview.chromium.org/276683005/diff/1/tools/telemetry/telemetry/co... tools/telemetry/telemetry/core/platform/profiler/win_pgo_profiler.py:18: nit; extra blank line here, and should be two lines at top level https://codereview.chromium.org/276683005/diff/1/tools/telemetry/telemetry/co... tools/telemetry/telemetry/core/platform/profiler/win_pgo_profiler.py:30: raise Exception('%s isn\'t in the current path, run vcvarsall.bat to fix ' this would normally be IOError, e.g. raise IOError(2, '%s isn\'t ...') https://codereview.chromium.org/276683005/diff/1/tools/telemetry/telemetry/co... tools/telemetry/telemetry/core/platform/profiler/win_pgo_profiler.py:38: def _GetNextProfileIndex(self, dll_name): is there something to clean up the pgc files? https://codereview.chromium.org/276683005/diff/1/tools/telemetry/telemetry/co... tools/telemetry/telemetry/core/platform/profiler/win_pgo_profiler.py:49: for pgc_file in pgc_files: maybe just max(max_index, int(os.path.splitext(os.path.split(pgc)[1])[0].split('!')[1])) (feel free to not) https://codereview.chromium.org/276683005/diff/1/tools/telemetry/telemetry/co... tools/telemetry/telemetry/core/platform/profiler/win_pgo_profiler.py:99: self._chrome_child_pgc_counter = self._chrome_child_pgc_counter + 1 += 1 seems easier to read https://codereview.chromium.org/276683005/diff/1/tools/telemetry/telemetry/co... tools/telemetry/telemetry/core/platform/profiler/win_pgo_profiler.py:104: self._chrome_pgc_counter = self._chrome_pgc_counter + 1 and here
Thanks Scott! https://codereview.chromium.org/276683005/diff/1/tools/telemetry/telemetry/co... File tools/telemetry/telemetry/core/platform/profiler/win_pgo_profiler.py (right): https://codereview.chromium.org/276683005/diff/1/tools/telemetry/telemetry/co... tools/telemetry/telemetry/core/platform/profiler/win_pgo_profiler.py:18: On 2014/05/08 16:56:27, scottmg wrote: > nit; extra blank line here, and should be two lines at top level Done. https://codereview.chromium.org/276683005/diff/1/tools/telemetry/telemetry/co... tools/telemetry/telemetry/core/platform/profiler/win_pgo_profiler.py:30: raise Exception('%s isn\'t in the current path, run vcvarsall.bat to fix ' On 2014/05/08 16:56:27, scottmg wrote: > this would normally be IOError, e.g. > > raise IOError(2, '%s isn\'t ...') Done. https://codereview.chromium.org/276683005/diff/1/tools/telemetry/telemetry/co... tools/telemetry/telemetry/core/platform/profiler/win_pgo_profiler.py:38: def _GetNextProfileIndex(self, dll_name): On 2014/05/08 16:56:27, scottmg wrote: > is there something to clean up the pgc files? This will be one of the build step. I don't want to do this here because we'll run several benchmarks during the PGO profiling step. https://codereview.chromium.org/276683005/diff/1/tools/telemetry/telemetry/co... tools/telemetry/telemetry/core/platform/profiler/win_pgo_profiler.py:49: for pgc_file in pgc_files: On 2014/05/08 16:56:27, scottmg wrote: > maybe just > > max(max_index, > int(os.path.splitext(os.path.split(pgc)[1])[0].split('!')[1])) > > > (feel free to not) Sweet ! https://codereview.chromium.org/276683005/diff/1/tools/telemetry/telemetry/co... tools/telemetry/telemetry/core/platform/profiler/win_pgo_profiler.py:99: self._chrome_child_pgc_counter = self._chrome_child_pgc_counter + 1 On 2014/05/08 16:56:27, scottmg wrote: > += 1 seems easier to read Shame on me... I'm always confused by the fact that there's no ++ operator but there's a += one... https://codereview.chromium.org/276683005/diff/1/tools/telemetry/telemetry/co... tools/telemetry/telemetry/core/platform/profiler/win_pgo_profiler.py:104: self._chrome_pgc_counter = self._chrome_pgc_counter + 1 On 2014/05/08 16:56:27, scottmg wrote: > and here Done.
ping ?
+tonyg@
lgtm in advance, but I do hope the FindSupportBinary method suggested will work. https://codereview.chromium.org/276683005/diff/20001/tools/telemetry/telemetr... File tools/telemetry/telemetry/core/platform/profiler/win_pgo_profiler.py (right): https://codereview.chromium.org/276683005/diff/20001/tools/telemetry/telemetr... tools/telemetry/telemetry/core/platform/profiler/win_pgo_profiler.py:25: pgosweep_is_in_path = False Just curious whether support_binaries.FindSupportBinary('pgosweep') works here. We might also consider uploading it to internal cloud storage. http://www.chromium.org/developers/telemetry/upload_to_cloud_storage https://codereview.chromium.org/276683005/diff/20001/tools/telemetry/telemetr... tools/telemetry/telemetry/core/platform/profiler/win_pgo_profiler.py:85: # profile data. No action necessary, but I do just idly wonder how much this hurts the quality of the profile.
Thanks! https://codereview.chromium.org/276683005/diff/20001/tools/telemetry/telemetr... File tools/telemetry/telemetry/core/platform/profiler/win_pgo_profiler.py (right): https://codereview.chromium.org/276683005/diff/20001/tools/telemetry/telemetr... tools/telemetry/telemetry/core/platform/profiler/win_pgo_profiler.py:25: pgosweep_is_in_path = False On 2014/05/17 08:49:42, tonyg wrote: > Just curious whether support_binaries.FindSupportBinary('pgosweep') works here. > We might also consider uploading it to internal cloud storage. > http://www.chromium.org/developers/telemetry/upload_to_cloud_storage It seems that FindSupportBinary looks for the binaries in the source tree, pgosweep.exe is a part of the toolchain (it comes with cl.exe, link.exe etc...). This is already in our internal cloud storage as a part of the win toolchain (checked in depot_tools). Also FindSupportBinary doesn't seem to be present in my checkout (part of a Chromium checkout), is this normal ? (It's also not on code search: https://code.google.com/p/chromium/codesearch#chromium/src/tools/telemetry/te...) https://codereview.chromium.org/276683005/diff/20001/tools/telemetry/telemetr... tools/telemetry/telemetry/core/platform/profiler/win_pgo_profiler.py:85: # profile data. On 2014/05/17 08:49:42, tonyg wrote: > No action necessary, but I do just idly wonder how much this hurts the quality > of the profile. This might have alter the data a little bit (e.g. we won't optimize the sandbox...) but we don't really have a choice here :(
The CQ bit was checked by sebmarchand@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sebmarchand@chromium.org/276683005/20001
Message was sent while issue was closed.
Change committed as 272293 |
