Issue 4808055: Script changes to include an optimize entry point for use by the chrome build...

Issue 4808055: Script changes to include an optimize entry point for use by the chrome build... (Closed)

Can't Edit
Can't Publish+Mail
Start Review

Created:
12 years, 9 months ago by Roger McFarlane

Modified:
12 years, 9 months ago

Reviewers:
chrisha, Siggi

CC:
sawbuck-changes_googlegroups.com

Base URL:
http://sawbuck.googlecode.com/svn/trunk/

Visibility:
Public.

Description

Benchmark package changes to include an "optimize" entry point for use by the chrome build bot. - when running optimize on a chrome build directory, we want to limit the number of extra files (particularly PDBs) we copy around. - Perform all work in/and on the copies of the original. - Add the ability to merge the chrome dll and pdb into the working copy from a different location. - Modify the relinker to write the new PDB to a temporary file then rename/replace it to the target file name. This allows the tool to rewrite a PDB file "in-place" (i.e., where source and destination filenames are the same). Committed: http://code.google.com/p/sawbuck/source/browse/#svn/trunk391

Patch Set 1 : '' #

Patch Set 2 : Ready for review. #

Total comments: 8

Patch Set 3 : Address initial comments. #

Total comments: 9

Patch Set 4 : address final nits #

Created: 12 years, 9 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+154 lines, -26 lines)			Patch
M	syzygy/relink/relinker.cc	View	1 2	2 chunks	+17 lines, -1 line	0 comments	Download
M	syzygy/scripts/benchmark/benchmark.gyp	View	1 2 3	1 chunk	+1 line, -0 lines	0 comments	Download
M	syzygy/scripts/benchmark/optimize.py	View	1 2 3	9 chunks	+114 lines, -16 lines	0 comments	Download
M	syzygy/scripts/benchmark/zip_benchmark.py	View	1 2	4 chunks	+22 lines, -9 lines	0 comments	Download

Messages

Total messages: 8

Expand All Messages | Collapse All Messages

Siggi

very cool! http://codereview.appspot.com/4808055/diff/7001/syzygy/scripts/benchmark/optimize.py File syzygy/scripts/benchmark/optimize.py (right): http://codereview.appspot.com/4808055/diff/7001/syzygy/scripts/benchmark/optimize.py#newcode112 syzygy/scripts/benchmark/optimize.py:112: # want will copy correctly, ignore the ...

12 years, 9 months ago (2011-07-27 09:39:17 UTC) #3

Roger McFarlane

PTAL. Note the change in relinker.cc http://codereview.appspot.com/4808055/diff/7001/syzygy/scripts/benchmark/optimize.py File syzygy/scripts/benchmark/optimize.py (right): http://codereview.appspot.com/4808055/diff/7001/syzygy/scripts/benchmark/optimize.py#newcode112 syzygy/scripts/benchmark/optimize.py:112: # want will ...

12 years, 9 months ago (2011-08-02 13:33:00 UTC) #4

PTAL.

Note the change in relinker.cc

http://codereview.appspot.com/4808055/diff/7001/syzygy/scripts/benchmark/opti...
File syzygy/scripts/benchmark/optimize.py (right):

http://codereview.appspot.com/4808055/diff/7001/syzygy/scripts/benchmark/opti...
syzygy/scripts/benchmark/optimize.py:112: # want will copy correctly, ignore the
exeption, and hope for the best
On 2011/07/27 09:39:17, Siggi wrote:
> It would make sense to log a warning here at least, so we can see what went
> wrong if this ever happens?

Done.

http://codereview.appspot.com/4808055/diff/7001/syzygy/scripts/benchmark/opti...
syzygy/scripts/benchmark/optimize.py:209: def _UpdateBinaries(original_dir,
optimized_dir):
On 2011/07/27 09:39:17, Siggi wrote:
> nit: I haven't read the other CLs so I don't know whether or how this is used,
> but replacing files as part of a build step is generally not a viable mode of
> operation.
> Eventually this will flake out, where either part of the replacing fails,
> leaving a mismatched or unmodified destination files, or else the modified
files
> are left in place and a non-clobber build comes along and rebuilds only some
of
> them...

Fair enough.

I have modified the build to generate the original chrome dll and pdb to an
intermediate location and then the optimize step copies the optimized versions
into place, so now the outputs mapped correctly.

http://codereview.appspot.com/4808055/diff/7001/syzygy/scripts/benchmark/opti...
syzygy/scripts/benchmark/optimize.py:212: _RmTree(backup_dir)
On 2011/07/27 09:39:17, Siggi wrote:
> note that _RmTree is best effort - as is it won't complain if it's unable to
> remove some or all of the files ...

the comment is now obsolete.

http://codereview.appspot.com/4808055/diff/7001/syzygy/scripts/benchmark/zip_...
File syzygy/scripts/benchmark/zip_benchmark.py (right):

http://codereview.appspot.com/4808055/diff/7001/syzygy/scripts/benchmark/zip_...
syzygy/scripts/benchmark/zip_benchmark.py:106: import optimize
On 2011/07/27 09:39:17, Siggi wrote:
> suggest using a single script wrapper and adding another template expansion
> parameter "%(module)s", as this is otherwise identical to the above?

Indeed. How embarrassing to have missed that.

Done.

chrisha

lgtm http://codereview.appspot.com/4808055/diff/12001/syzygy/relink/relinker.cc File syzygy/relink/relinker.cc (right): http://codereview.appspot.com/4808055/diff/12001/syzygy/relink/relinker.cc#newcode577 syzygy/relink/relinker.cc:577: file_util::Delete(temp_pdb, false); How fast is ReplaceFile? It might ...

12 years, 9 months ago (2011-08-02 13:56:15 UTC) #5

Siggi

lgtm http://codereview.appspot.com/4808055/diff/12001/syzygy/scripts/benchmark/optimize.py File syzygy/scripts/benchmark/optimize.py (right): http://codereview.appspot.com/4808055/diff/12001/syzygy/scripts/benchmark/optimize.py#newcode262 syzygy/scripts/benchmark/optimize.py:262: 'installation be created. From this location, ' nit: ...

12 years, 9 months ago (2011-08-02 14:01:53 UTC) #6

Roger McFarlane

Thanks. Committed. http://codereview.appspot.com/4808055/diff/12001/syzygy/relink/relinker.cc File syzygy/relink/relinker.cc (right): http://codereview.appspot.com/4808055/diff/12001/syzygy/relink/relinker.cc#newcode577 syzygy/relink/relinker.cc:577: file_util::Delete(temp_pdb, false); On 2011/08/02 13:56:15, chrisha wrote: ...

12 years, 9 months ago (2011-08-02 14:18:56 UTC) #7

Siggi

12 years, 9 months ago (2011-08-02 14:30:51 UTC) #8

On Tue, Aug 2, 2011 at 10:18 AM, <rogerm@chromium.org> wrote:

> Thanks.
>
> Committed.
>
>
>
> http://codereview.appspot.com/**4808055/diff/12001/syzygy/**
>
relink/relinker.cc<http://codereview.appspot.com/4808055/diff/12001/syzygy/relink/relinker.cc>
> File syzygy/relink/relinker.cc (right):
>
> http://codereview.appspot.com/**4808055/diff/12001/syzygy/**
>
relink/relinker.cc#newcode577<http://codereview.appspot.com/4808055/diff/12001/syzygy/relink/relinker.cc#newcode577>
> syzygy/relink/relinker.cc:577: file_util::Delete(temp_pdb, false);
> On 2011/08/02 13:56:15, chrisha wrote:
>
>> How fast is ReplaceFile? It might be worthwhile making the 'via
>>
> temporary file'
>
>> mechanism only run if output_path == input_path.
>>
>
> It seems pretty quick.  Would need to add some logging/timing info to be
> sure, but subjectively, I haven't noticed anything.
>
> Because I'm doing the replace on the same volume, I think the operations
> it's doing are:
>
>  rename original away
>  rename ole file to original
>  delete renamed original
>
> The overriding rename-and-delete is atomic for all sane file systems :).


> i.e., I dont' think there's a copy taking place.  IF it was across
> volumes, then there would be a copy.
>
>
> http://codereview.appspot.com/**4808055/diff/12001/syzygy/**
>
scripts/benchmark/optimize.py<http://codereview.appspot.com/4808055/diff/12001/syzygy/scripts/benchmark/optimize.py>
> File syzygy/scripts/benchmark/**optimize.py (right):
>
> http://codereview.appspot.com/**4808055/diff/12001/syzygy/**
>
scripts/benchmark/optimize.py#**newcode198<http://codereview.appspot.com/4808055/diff/12001/syzygy/scripts/benchmark/optimize.py#newcode198>
> syzygy/scripts/benchmark/**optimize.py:198: or os.path.join(chrome_dir,
> '\chrome.dll')),
> On 2011/08/02 13:56:15, chrisha wrote:
>
>> Is it just me, or would this read better if using the ternary
>>
> operator?
>
>  input_dll if input_dll else os.path.join(...)
>>
>
> Fair enough.
>
> Done.
>
>
> http://codereview.appspot.com/**4808055/diff/12001/syzygy/**
>
scripts/benchmark/optimize.py#**newcode262<http://codereview.appspot.com/4808055/diff/12001/syzygy/scripts/benchmark/optimize.py#newcode262>
> syzygy/scripts/benchmark/**optimize.py:262: 'installation be created. From
> this location, '
> On 2011/08/02 13:56:15, chrisha wrote:
>
>> will* be created
>>
>
> Done.
>
>
> http://codereview.appspot.com/**4808055/diff/12001/syzygy/**
>
scripts/benchmark/zip_**benchmark.py<http://codereview.appspot.com/4808055/diff/12001/syzygy/scripts/benchmark/zip_benchmark.py>
> File syzygy/scripts/benchmark/zip_**benchmark.py (right):
>
> http://codereview.appspot.com/**4808055/diff/12001/syzygy/**
>
scripts/benchmark/zip_**benchmark.py#newcode77<http://codereview.appspot.com/4808055/diff/12001/syzygy/scripts/benchmark/zip_benchmark.py#newcode77>
> syzygy/scripts/benchmark/zip_**benchmark.py:77: ('optimize.bat',
> _SCRIPT_TEMPLATE, 'optimize'),
> On 2011/08/02 14:01:53, Siggi wrote:
>
>> do you need to add this file to the build action output files in the
>>
> GYP file?
>
> Done.
>
>
>
http://codereview.appspot.com/**4808055/<http://codereview.appspot.com/4808055/>
>

Expand All Messages | Collapse All Messages