Automate the process of optimizing and benchmarking Chrome.

Automate the process of optimizing and benchmarking Chrome.
Committed: http://code.google.com/p/sawbuck/source/browse/#svn/trunk379

[chrisha, 2011-07-07 14:22:29]

lgtm, with a few questions.

http://codereview.appspot.com/4631051/diff/16001/syzygy/scripts/benchmark/opt...
File syzygy/scripts/benchmark/optimize.py (right):

http://codereview.appspot.com/4631051/diff/16001/syzygy/scripts/benchmark/opt...
syzygy/scripts/benchmark/optimize.py:78:
self._call_trace_logger.EnableProvider(_CALL_TRACE_PROVIDER,
All of this could be done using 'call_trace_control', that way there's only once
place where code lives that does startup and teardown of ETW log sessions.

http://codereview.appspot.com/4631051/diff/16001/syzygy/scripts/benchmark/opt...
syzygy/scripts/benchmark/optimize.py:85: self._log_files.append(kernel_file)
Didn't you mention that KernelLogger needs to be started first?

http://codereview.appspot.com/4631051/diff/16001/syzygy/scripts/benchmark/opt...
syzygy/scripts/benchmark/optimize.py:126: shutil.copy(src_file, dst_file)
Hmm... shouldn't failure to instrument be the death of the whole script?
Profiling won't return anything, and the rest of the work become meaningless,
no?

http://codereview.appspot.com/4631051/diff/16001/syzygy/scripts/benchmark/run...
File syzygy/scripts/benchmark/runner.py (right):

http://codereview.appspot.com/4631051/diff/16001/syzygy/scripts/benchmark/run...
syzygy/scripts/benchmark/runner.py:115: p.contents.MinimumBuffers = 100
Once again, we could be using call_trace_control to minimize duplication of
code.

[Roger McFarlane, 2011-07-07 14:58:16]

http://codereview.appspot.com/4631051/diff/16001/syzygy/scripts/benchmark/opt...
File syzygy/scripts/benchmark/optimize.py (right):

http://codereview.appspot.com/4631051/diff/16001/syzygy/scripts/benchmark/opt...
syzygy/scripts/benchmark/optimize.py:15: from etw.guiddef import GUID
docstring

http://codereview.appspot.com/4631051/diff/16001/syzygy/scripts/benchmark/opt...
syzygy/scripts/benchmark/optimize.py:125: file)
raise an exception here?

http://codereview.appspot.com/4631051/diff/16001/syzygy/scripts/benchmark/opt...
syzygy/scripts/benchmark/optimize.py:126: shutil.copy(src_file, dst_file)
Is this indented one level too many?

http://codereview.appspot.com/4631051/diff/16001/syzygy/scripts/benchmark/opt...
syzygy/scripts/benchmark/optimize.py:228: os.path.join(temp_dir,
'call_trace.etl'),]
back-to-back assignment to trace_files?

http://codereview.appspot.com/4631051/diff/16001/syzygy/scripts/benchmark/opt...
syzygy/scripts/benchmark/optimize.py:230: except:
This except clause is unnecessary, you'll get equivalent behavior by default

http://codereview.appspot.com/4631051/diff/16001/syzygy/scripts/benchmark/opt...
syzygy/scripts/benchmark/optimize.py:244: sys.exit(main())
no need for the sys.exit, you can just call main() and drop the return 0 from
the end of main.  On error you exit via an exception, which will exit 1.

http://codereview.appspot.com/4631051/diff/16001/syzygy/scripts/benchmark/run...
File syzygy/scripts/benchmark/runner.py (right):

http://codereview.appspot.com/4631051/diff/16001/syzygy/scripts/benchmark/run...
syzygy/scripts/benchmark/runner.py:115: p.contents.MinimumBuffers = 100
On 2011/07/07 14:22:29, chrisha wrote:
> Once again, we could be using call_trace_control to minimize duplication of
> code.

We could further have call_trace_control export python entrypoints.

[Siggi, 2011-07-07 18:30:26]

Thanks, PTAL.

http://codereview.appspot.com/4631051/diff/16001/syzygy/scripts/benchmark/opt...
File syzygy/scripts/benchmark/optimize.py (right):

http://codereview.appspot.com/4631051/diff/16001/syzygy/scripts/benchmark/opt...
syzygy/scripts/benchmark/optimize.py:15: from etw.guiddef import GUID
On 2011/07/07 14:58:16, Roger McFarlane wrote:
> docstring

Done.

http://codereview.appspot.com/4631051/diff/16001/syzygy/scripts/benchmark/opt...
syzygy/scripts/benchmark/optimize.py:78:
self._call_trace_logger.EnableProvider(_CALL_TRACE_PROVIDER,
On 2011/07/07 14:22:29, chrisha wrote:
> All of this could be done using 'call_trace_control', that way there's only
once
> place where code lives that does startup and teardown of ETW log sessions.

Done.

http://codereview.appspot.com/4631051/diff/16001/syzygy/scripts/benchmark/opt...
syzygy/scripts/benchmark/optimize.py:85: self._log_files.append(kernel_file)
On 2011/07/07 14:22:29, chrisha wrote:
> Didn't you mention that KernelLogger needs to be started first?

I forget, this is the order that seems to work.

http://codereview.appspot.com/4631051/diff/16001/syzygy/scripts/benchmark/opt...
syzygy/scripts/benchmark/optimize.py:125: file)
On 2011/07/07 14:58:16, Roger McFarlane wrote:
> raise an exception here?

Done.

http://codereview.appspot.com/4631051/diff/16001/syzygy/scripts/benchmark/opt...
syzygy/scripts/benchmark/optimize.py:126: shutil.copy(src_file, dst_file)
On 2011/07/07 14:22:29, chrisha wrote:
> Hmm... shouldn't failure to instrument be the death of the whole script?
> Profiling won't return anything, and the rest of the work become meaningless,
> no?

Done.

http://codereview.appspot.com/4631051/diff/16001/syzygy/scripts/benchmark/opt...
syzygy/scripts/benchmark/optimize.py:126: shutil.copy(src_file, dst_file)
On 2011/07/07 14:58:16, Roger McFarlane wrote:
> Is this indented one level too many?

Done.

http://codereview.appspot.com/4631051/diff/16001/syzygy/scripts/benchmark/opt...
syzygy/scripts/benchmark/optimize.py:228: os.path.join(temp_dir,
'call_trace.etl'),]
On 2011/07/07 14:58:16, Roger McFarlane wrote:
> back-to-back assignment to trace_files?

Done.

http://codereview.appspot.com/4631051/diff/16001/syzygy/scripts/benchmark/opt...
syzygy/scripts/benchmark/optimize.py:230: except:
On 2011/07/07 14:58:16, Roger McFarlane wrote:
> This except clause is unnecessary, you'll get equivalent behavior by default

True.

http://codereview.appspot.com/4631051/diff/16001/syzygy/scripts/benchmark/opt...
syzygy/scripts/benchmark/optimize.py:244: sys.exit(main())
On 2011/07/07 14:58:16, Roger McFarlane wrote:
> no need for the sys.exit, you can just call main() and drop the return 0 from
> the end of main.  On error you exit via an exception, which will exit 1.

I don't care particularly how we do this, but we should be consistent over
scripts in Syzygy. A code search shows we use this idiom pretty consistently.

http://codereview.appspot.com/4631051/diff/16001/syzygy/scripts/benchmark/run...
File syzygy/scripts/benchmark/runner.py (right):

http://codereview.appspot.com/4631051/diff/16001/syzygy/scripts/benchmark/run...
syzygy/scripts/benchmark/runner.py:115: p.contents.MinimumBuffers = 100
On 2011/07/07 14:22:29, chrisha wrote:
> Once again, we could be using call_trace_control to minimize duplication of
> code.

Done.

http://codereview.appspot.com/4631051/diff/16001/syzygy/scripts/benchmark/run...
syzygy/scripts/benchmark/runner.py:115: p.contents.MinimumBuffers = 100
On 2011/07/07 14:58:16, Roger McFarlane wrote:
> On 2011/07/07 14:22:29, chrisha wrote:
> > Once again, we could be using call_trace_control to minimize duplication of
> > code.
> 
> We could further have call_trace_control export python entrypoints. 

This is actually a standalone control executable Chris wrote.

[Roger McFarlane, 2011-07-07 20:07:04]

lgtm

[chrisha, 2011-07-07 20:07:19]

lgtm