Telemetry: add a metadata layer between page set and .wpr.

Telemetry: add a metadata layer between page set and .wpr.

The metadata file describes which pages in the page set are
backed by which .wpr files. This allows us to update individual
pages in the page set.

BUG=155660
NOTRY=true


Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=180117

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=180133

[marja, 2013-01-16 13:05:05]

Hi nduca@,

Here's the first version of the metadata layer thing, can you check if this is
going to the right direction?

The corresponding index.json file:
{
    "page_set": "../../page_sets/top_25.json", 
    "description": "Describes the Web Page Replay archives for a page set. Don't
edit by hand! Use record_wpr for updating.", 
    "archives": {
        "top_25_00.wpr": [
           
"https://docs.google.com/document/d/1X-IKNjtEnx-WW5JIKRLsyhz5sbsat3mfTpAPUSX3_s4/view",

            "http://www.youtube.com", 
            "http://news.yahoo.com", 
            "http://www.linkedin.com/in/linustorvalds", 
            "http://www.ebay.com", 
            "http://answers.yahoo.com", 
            "http://www.cnn.com", 
            "http://www.weather.com/weather/right-now/Mountain+View+CA+94043", 
            "http://espn.go.com", 
            "http://en.wikipedia.org/wiki/Wikipedia", 
            "http://games.yahoo.com", 
            "https://www.google.com/#hl=en&q=barack+obama", 
            "http://booking.com", 
            "http://pinterest.com", 
            "http://sports.yahoo.com/", 
            "https://www.google.com/calendar/", 
            "http://twitter.com/katyperry", 
            "https://drive.google.com", 
            "http://www.amazon.com", 
            "http://googlewebmastercentral.blogspot.com/", 
           
"http://en.blog.wordpress.com/2012/09/04/freshly-pressed-editors-picks-for-august-2012/",

            "https://plus.google.com/110031535020051778989/posts", 
            "http://techcrunch.com"
        ], 
        "top_25_01.wpr": [
            "https://mail.google.com/mail/"
        ], 
        "top_25_002.wpr": [
            "http://www.facebook.com/barackobama"
        ]
    }
}

[marja, 2013-01-21 11:45:51]

A friendly ping.

[marja, 2013-01-23 14:30:17]

Hi dtu@, nduca@ suggested you do the first review pass. Can you have a look?

[dtu, 2013-01-23 21:07:02]

https://codereview.chromium.org/11881051/diff/8001/tools/perf/page_sets/top_2...
File tools/perf/page_sets/top_25.json (right):

https://codereview.chromium.org/11881051/diff/8001/tools/perf/page_sets/top_2...
tools/perf/page_sets/top_25.json:3: "archive_data_file":
"../data/top_25/index.json",
Are you updating all the page sets to use the new format?

https://codereview.chromium.org/11881051/diff/8001/tools/telemetry/telemetry/...
File tools/telemetry/telemetry/page_runner.py (right):

https://codereview.chromium.org/11881051/diff/8001/tools/telemetry/telemetry/...
tools/telemetry/telemetry/page_runner.py:117: if options.wpr_mode ==
wpr_modes.WPR_OFF:
I think "!= wpr_modes.WPR_RECORD" is clearer

https://codereview.chromium.org/11881051/diff/8001/tools/telemetry/telemetry/...
File tools/telemetry/telemetry/page_set.py (right):

https://codereview.chromium.org/11881051/diff/8001/tools/telemetry/telemetry/...
tools/telemetry/telemetry/page_set.py:15: self.archive_data_file = ''
It's more useful to keep track of archive_data_dir (with the base_dir included),
since you use that several times here and in record_wpr.

https://codereview.chromium.org/11881051/diff/8001/tools/telemetry/telemetry/...
tools/telemetry/telemetry/page_set.py:20: self.wpr_data_per_urls = dict()
url_to_wpr_file. Note that these wpr file paths are absolute paths, not the same
as written in the archive data file.

https://codereview.chromium.org/11881051/diff/8001/tools/telemetry/telemetry/...
tools/telemetry/telemetry/page_set.py:21: self.wpr_data_per_wpr_files = dict()
wpr_file_to_url. Note that these wpr file paths are relative to the archive data
file, exactly as written in the data file.

https://codereview.chromium.org/11881051/diff/8001/tools/telemetry/telemetry/...
tools/telemetry/telemetry/page_set.py:34: wpr_data = json.loads(contents)
json.load() to read from the fp directly

https://codereview.chromium.org/11881051/diff/8001/tools/telemetry/telemetry/...
tools/telemetry/telemetry/page_set.py:38: page_urls =
wpr_data['archives'][wpr_file]
Are you sure this is right? A for loop in Python loops through the elements of
the list, not their indices.

https://codereview.chromium.org/11881051/diff/8001/tools/telemetry/telemetry/...
tools/telemetry/telemetry/page_set.py:42: self.base_dir, archive_data_dir,
wpr_file)))
You already included base_dir in archive_data_dir.

https://codereview.chromium.org/11881051/diff/8001/tools/telemetry/telemetry/...
tools/telemetry/telemetry/page_set.py:106: def FilterPages(self, options):
Prefer if this takes just the filter string instead of the entire options
object.

https://codereview.chromium.org/11881051/diff/8001/tools/telemetry/telemetry/...
File tools/telemetry/telemetry/page_set_unittest.py (right):

https://codereview.chromium.org/11881051/diff/8001/tools/telemetry/telemetry/...
tools/telemetry/telemetry/page_set_unittest.py:11: "archives": []
Can you have a test with something in the archives?

https://codereview.chromium.org/11881051/diff/8001/tools/telemetry/telemetry/...
File tools/telemetry/telemetry/record_wpr.py (right):

https://codereview.chromium.org/11881051/diff/8001/tools/telemetry/telemetry/...
tools/telemetry/telemetry/record_wpr.py:89: page_urls =
ps.wpr_data_per_wpr_files[wpr_file]
for wpr_file, page_urls in ps.wpr_data_per_wpr_files.iteritems():

https://codereview.chromium.org/11881051/diff/8001/tools/telemetry/telemetry/...
tools/telemetry/telemetry/record_wpr.py:95: wpr_files_not_needed =
wpr_files_not_needed[1:]
I don't think it's useful to reuse the old filename.

https://codereview.chromium.org/11881051/diff/8001/tools/telemetry/telemetry/...
tools/telemetry/telemetry/record_wpr.py:140: temp_target_wpr_path =
target_wpr_path + '.temp'
temp_target_wpr_path = tempfile.mkstemp()[1]

[marja, 2013-01-24 16:03:33]

Thanks for comments!

I simplified this, by adding a separate class for the metadata. That takes care
of the updating logic, and I also added unit tests for it.

https://codereview.chromium.org/11881051/diff/8001/tools/perf/page_sets/top_2...
File tools/perf/page_sets/top_25.json (right):

https://codereview.chromium.org/11881051/diff/8001/tools/perf/page_sets/top_2...
tools/perf/page_sets/top_25.json:3: "archive_data_file":
"../data/top_25/index.json",
On 2013/01/23 21:07:02, Dave Tu wrote:
> Are you updating all the page sets to use the new format?

Yes, I'll need to add a similar kind of update to other pages sets in this CL,
because the old format won't work any more.

Also, I need to update the .wprs to have new file names. So I need these CLs:
1. src-internal, copy .wprs under new names, add metadata files
2. This CL
3. src-internal, remove old .wpr files

(Added the other .json file updates to this CL. I hadn't done that yet, since I
hadn't finalized the data directory layout. Now all the data will be in ../data,
and the json files will have the file names "dataset.json". All the data can
still be in the same directory...)

https://codereview.chromium.org/11881051/diff/8001/tools/telemetry/telemetry/...
File tools/telemetry/telemetry/page_runner.py (right):

https://codereview.chromium.org/11881051/diff/8001/tools/telemetry/telemetry/...
tools/telemetry/telemetry/page_runner.py:117: if options.wpr_mode ==
wpr_modes.WPR_OFF:
On 2013/01/23 21:07:02, Dave Tu wrote:
> I think "!= wpr_modes.WPR_RECORD" is clearer

Done.

https://codereview.chromium.org/11881051/diff/8001/tools/telemetry/telemetry/...
File tools/telemetry/telemetry/page_set.py (right):

https://codereview.chromium.org/11881051/diff/8001/tools/telemetry/telemetry/...
tools/telemetry/telemetry/page_set.py:15: self.archive_data_file = ''
On 2013/01/23 21:07:02, Dave Tu wrote:
> It's more useful to keep track of archive_data_dir (with the base_dir
included),
> since you use that several times here and in record_wpr.

Done.

https://codereview.chromium.org/11881051/diff/8001/tools/telemetry/telemetry/...
tools/telemetry/telemetry/page_set.py:20: self.wpr_data_per_urls = dict()
On 2013/01/23 21:07:02, Dave Tu wrote:
> url_to_wpr_file. Note that these wpr file paths are absolute paths, not the
same
> as written in the archive data file.

Done.

Also, I changed these to be relative paths, so that juggling with the two maps
is easier.

https://codereview.chromium.org/11881051/diff/8001/tools/telemetry/telemetry/...
tools/telemetry/telemetry/page_set.py:21: self.wpr_data_per_wpr_files = dict()
On 2013/01/23 21:07:02, Dave Tu wrote:
> wpr_file_to_url. Note that these wpr file paths are relative to the archive
data
> file, exactly as written in the data file.

Done.

https://codereview.chromium.org/11881051/diff/8001/tools/telemetry/telemetry/...
tools/telemetry/telemetry/page_set.py:34: wpr_data = json.loads(contents)
On 2013/01/23 21:07:02, Dave Tu wrote:
> json.load() to read from the fp directly

Done.

https://codereview.chromium.org/11881051/diff/8001/tools/telemetry/telemetry/...
tools/telemetry/telemetry/page_set.py:38: page_urls =
wpr_data['archives'][wpr_file]
On 2013/01/23 21:07:02, Dave Tu wrote:
> Are you sure this is right? A for loop in Python loops through the elements of
> the list, not their indices.

wpr_data['archives'] is a dictionary from wpr file name to a list of urls (see
the metadata example in the CL message thread).

https://codereview.chromium.org/11881051/diff/8001/tools/telemetry/telemetry/...
tools/telemetry/telemetry/page_set.py:42: self.base_dir, archive_data_dir,
wpr_file)))
On 2013/01/23 21:07:02, Dave Tu wrote:
> You already included base_dir in archive_data_dir.

This snippet is gone, because I made the map contain relative wpr file names.

https://codereview.chromium.org/11881051/diff/8001/tools/telemetry/telemetry/...
tools/telemetry/telemetry/page_set.py:106: def FilterPages(self, options):
On 2013/01/23 21:07:02, Dave Tu wrote:
> Prefer if this takes just the filter string instead of the entire options
> object.

This will change after my other CL ( https://codereview.chromium.org/12034007/ )
gets in, I'll rebase after that.

Basically, I'll have a PageFilter object which will encapsulate this logic.

https://codereview.chromium.org/11881051/diff/8001/tools/telemetry/telemetry/...
File tools/telemetry/telemetry/page_set_unittest.py (right):

https://codereview.chromium.org/11881051/diff/8001/tools/telemetry/telemetry/...
tools/telemetry/telemetry/page_set_unittest.py:11: "archives": []
On 2013/01/23 21:07:02, Dave Tu wrote:
> Can you have a test with something in the archives?

I added a test here for testing that the archive paths for pages are read
properly, and I added a test for the new metadata class.

https://codereview.chromium.org/11881051/diff/8001/tools/telemetry/telemetry/...
File tools/telemetry/telemetry/record_wpr.py (right):

https://codereview.chromium.org/11881051/diff/8001/tools/telemetry/telemetry/...
tools/telemetry/telemetry/record_wpr.py:89: page_urls =
ps.wpr_data_per_wpr_files[wpr_file]
On 2013/01/23 21:07:02, Dave Tu wrote:
> for wpr_file, page_urls in ps.wpr_data_per_wpr_files.iteritems():

Done.

https://codereview.chromium.org/11881051/diff/8001/tools/telemetry/telemetry/...
tools/telemetry/telemetry/record_wpr.py:95: wpr_files_not_needed =
wpr_files_not_needed[1:]
On 2013/01/23 21:07:02, Dave Tu wrote:
> I don't think it's useful to reuse the old filename.

Done.

https://codereview.chromium.org/11881051/diff/8001/tools/telemetry/telemetry/...
tools/telemetry/telemetry/record_wpr.py:140: temp_target_wpr_path =
target_wpr_path + '.temp'
On 2013/01/23 21:07:02, Dave Tu wrote:
> temp_target_wpr_path = tempfile.mkstemp()[1]

Done.

[dtu, 2013-01-25 00:22:30]

Nice, I like this separate metadata class.

https://chromiumcodereview.appspot.com/11881051/diff/20031/tools/telemetry/te...
File tools/telemetry/telemetry/page_set_metadata.py (right):

https://chromiumcodereview.appspot.com/11881051/diff/20031/tools/telemetry/te...
tools/telemetry/telemetry/page_set_metadata.py:27: self.url_to_wpr_file[url] =
wpr_file
All of these members are private, right? Precede with underscore.

https://chromiumcodereview.appspot.com/11881051/diff/20031/tools/telemetry/te...
tools/telemetry/telemetry/page_set_metadata.py:32: return None
return self.url_to_wpr_file.get(page.url, None)

https://chromiumcodereview.appspot.com/11881051/diff/20031/tools/telemetry/te...
tools/telemetry/telemetry/page_set_metadata.py:74: return
[self._WprFileNameToPath(wpr_file) for wpr_file in wpr_files]
Delete the files here instead of returning the list of files to be deleted.

https://chromiumcodereview.appspot.com/11881051/diff/20031/tools/telemetry/te...
tools/telemetry/telemetry/page_set_metadata.py:89: f.write(json.dumps(metadata,
indent=4))
Just json.dump(metadata, indent=4)

https://chromiumcodereview.appspot.com/11881051/diff/20031/tools/telemetry/te...
File tools/telemetry/telemetry/record_wpr.py (right):

https://chromiumcodereview.appspot.com/11881051/diff/20031/tools/telemetry/te...
tools/telemetry/telemetry/record_wpr.py:84: # after the recording.
Seems like the only thing you need before recording is setting a temporary wpr
file. You can move all these metadata things after the recording. Then maybe
wrap them all up into a function?

[nduca, 2013-01-25 10:34:06]

This looks really good!

i think maybe page_set_metdata is the wrong word, its hard to explain to random
people on the street what it does. Not sure better word, though. WPRArchiveMap?
ArchiveMap? I dont know... something that says what it does. PageSetArchive?

https://codereview.chromium.org/11881051/diff/20031/tools/telemetry/telemetry...
File tools/telemetry/telemetry/multi_page_benchmark_runner.py (right):

https://codereview.chromium.org/11881051/diff/20031/tools/telemetry/telemetry...
tools/telemetry/telemetry/multi_page_benchmark_runner.py:66:
ps.FilterPages(options)
why'd filtering move?

https://codereview.chromium.org/11881051/diff/20031/tools/telemetry/telemetry...
File tools/telemetry/telemetry/multi_page_benchmark_unittest.py (right):

https://codereview.chromium.org/11881051/diff/20031/tools/telemetry/telemetry...
tools/telemetry/telemetry/multi_page_benchmark_unittest.py:94:
ps.pages[0].archive_path = test_archive
this feels funky. We should probably be constructing the full metadata.

https://codereview.chromium.org/11881051/diff/20031/tools/telemetry/telemetry...
File tools/telemetry/telemetry/page_set.py (right):

https://codereview.chromium.org/11881051/diff/20031/tools/telemetry/telemetry...
tools/telemetry/telemetry/page_set.py:15: self.description = ''
it seems to me that if we instead require the constructor for a pageset here to
pass in a metadata object, then we dont have to mess with this function much.
FromFile can do the stuff about making the making the class

https://codereview.chromium.org/11881051/diff/20031/tools/telemetry/telemetry...
tools/telemetry/telemetry/page_set.py:29: self.wpr_metadata =
page_set_metadata.PageSetMetadata(
maybe we should make it possible to construct a page_set with a metadata object
directly for unittest purposes?

https://codereview.chromium.org/11881051/diff/20031/tools/telemetry/telemetry...
tools/telemetry/telemetry/page_set.py:49: page.archive_path =
page_set.wpr_metadata.WprFilePathForPage(page)
instead of stashing this on page, lets just have a lookup method on the
page_set: "GetArchivePathForPage(page)"?

https://codereview.chromium.org/11881051/diff/20031/tools/telemetry/telemetry...
File tools/telemetry/telemetry/page_set_metadata.py (right):

https://codereview.chromium.org/11881051/diff/20031/tools/telemetry/telemetry...
tools/telemetry/telemetry/page_set_metadata.py:10: def __init__(self,
archive_data_file_path):
I think this should probably follow the same template as Page_set. Plain
constrcturo, @fromFile for loading

https://codereview.chromium.org/11881051/diff/20031/tools/telemetry/telemetry...
File tools/telemetry/telemetry/record_wpr.py (right):

https://codereview.chromium.org/11881051/diff/20031/tools/telemetry/telemetry...
tools/telemetry/telemetry/record_wpr.py:91: wpr_files_not_needed =
ps.wpr_metadata.RemoveAbandonedWprFiles()
the interface on the wpr_metadata seems to mix side effects with retrieval. Can
you make it a bit more action-vs-access?

GetListOfAbandonedWprFiles() # side effect free
metadata.DidRenameWprFile(oldName,newName)
etc?

https://codereview.chromium.org/11881051/diff/20031/tools/telemetry/telemetry...
tools/telemetry/telemetry/record_wpr.py:120: if not results.page_failures:
what if someday 5 pages succeed and 1 fails. How hard will it be to make the
code written today support better handling of that case? Its a good way to test
the cleanliness of the current patch. I think the architecutre isn't quite ready
for that, which means it needs refactoring --- not to actually implement what I
describe, but not require heavy refactoring.

I think the key is that we can write out the metadata at any time, and that as
we successfully record a page, we update which archive it points at. And, if we
wanted, write the metadata at that point. As we do that, old archives will
implicitly become orphaned and they can be deleted, or we can simply scavange
them at the end [ps.wpr_metadata.unused_files()]

https://codereview.chromium.org/11881051/diff/20031/tools/telemetry/telemetry...
tools/telemetry/telemetry/record_wpr.py:122:
ps.wpr_metadata.WriteToFile(os.path.abspath(
what if any of the calls here fail?

https://codereview.chromium.org/11881051/diff/20031/tools/telemetry/telemetry...
tools/telemetry/telemetry/record_wpr.py:124: for wpr_file_path in
wpr_files_not_needed:
i am kind of worried about the ordering of the deletion calls here relative to
the RemoveAbandonedWprFiles above. As designed, the metadata and filesystem are
in inconsistent states i think up until the end here. Can you figure out a way
to make it continually consistent?

[marja, 2013-01-28 17:48:03]

Thanks again for comments!

In addition to the "done"s here, I renamed PageSetMetadata to
PageSetArchiveInfo.

https://codereview.chromium.org/11881051/diff/20031/tools/telemetry/telemetry...
File tools/telemetry/telemetry/multi_page_benchmark_runner.py (right):

https://codereview.chromium.org/11881051/diff/20031/tools/telemetry/telemetry...
tools/telemetry/telemetry/multi_page_benchmark_runner.py:66:
ps.FilterPages(options)
On 2013/01/25 10:34:06, nduca wrote:
> why'd filtering move?

We need to know here which pages will be ran, so that we can update the metadata
for those pages.

But, if we want to keep the filtering behind PageRunner... I added a param to
the PageRunner.run() so that it gives information about the pages it ran.

https://codereview.chromium.org/11881051/diff/20031/tools/telemetry/telemetry...
File tools/telemetry/telemetry/multi_page_benchmark_unittest.py (right):

https://codereview.chromium.org/11881051/diff/20031/tools/telemetry/telemetry...
tools/telemetry/telemetry/multi_page_benchmark_unittest.py:94:
ps.pages[0].archive_path = test_archive
On 2013/01/25 10:34:06, nduca wrote:
> this feels funky. We should probably be constructing the full metadata.

Done. (I didn't write the metadata to a file but construct it based on a string.
The actual data is not written to a file either, and pages are not constructed
based on it, but tweaked manually.)

https://codereview.chromium.org/11881051/diff/20031/tools/telemetry/telemetry...
File tools/telemetry/telemetry/page_set.py (right):

https://codereview.chromium.org/11881051/diff/20031/tools/telemetry/telemetry...
tools/telemetry/telemetry/page_set.py:15: self.description = ''
On 2013/01/25 10:34:06, nduca wrote:
> it seems to me that if we instead require the constructor for a pageset here
to
> pass in a metadata object, then we dont have to mess with this function much.
> FromFile can do the stuff about making the making the class 

How about this version which reads the filename from the attributes, and then
calls PageSetMetadata.FromFile?

https://codereview.chromium.org/11881051/diff/20031/tools/telemetry/telemetry...
tools/telemetry/telemetry/page_set.py:29: self.wpr_metadata =
page_set_metadata.PageSetMetadata(
On 2013/01/25 10:34:06, nduca wrote:
> maybe we should make it possible to construct a page_set with a metadata
object
> directly for unittest purposes?

I made the PageSetMetadata ctor callable in unit tests.

https://codereview.chromium.org/11881051/diff/20031/tools/telemetry/telemetry...
tools/telemetry/telemetry/page_set.py:49: page.archive_path =
page_set.wpr_metadata.WprFilePathForPage(page)
On 2013/01/25 10:34:06, nduca wrote:
> instead of stashing this on page, lets just have a lookup method on the
> page_set: "GetArchivePathForPage(page)"?

In that case, when at some point we only have a Page which we try to run, we
would go back to the PageSet to find out the archive data file... I'd rather
store the information in the Page itself.

https://codereview.chromium.org/11881051/diff/20031/tools/telemetry/telemetry...
File tools/telemetry/telemetry/page_set_metadata.py (right):

https://codereview.chromium.org/11881051/diff/20031/tools/telemetry/telemetry...
tools/telemetry/telemetry/page_set_metadata.py:10: def __init__(self,
archive_data_file_path):
On 2013/01/25 10:34:06, nduca wrote:
> I think this should probably follow the same template as Page_set. Plain
> constrcturo, @fromFile for loading

Done.

https://codereview.chromium.org/11881051/diff/20031/tools/telemetry/telemetry...
tools/telemetry/telemetry/page_set_metadata.py:27: self.url_to_wpr_file[url] =
wpr_file
On 2013/01/25 00:22:31, Dave Tu wrote:
> All of these members are private, right? Precede with underscore.

Done.

https://codereview.chromium.org/11881051/diff/20031/tools/telemetry/telemetry...
tools/telemetry/telemetry/page_set_metadata.py:32: return None
On 2013/01/25 00:22:31, Dave Tu wrote:
> return self.url_to_wpr_file.get(page.url, None)

Done.

https://codereview.chromium.org/11881051/diff/20031/tools/telemetry/telemetry...
tools/telemetry/telemetry/page_set_metadata.py:74: return
[self._WprFileNameToPath(wpr_file) for wpr_file in wpr_files]
On 2013/01/25 00:22:31, Dave Tu wrote:
> Delete the files here instead of returning the list of files to be deleted.

I added a func which only reads the abandoned files, and another one which
deletes them too.

https://codereview.chromium.org/11881051/diff/20031/tools/telemetry/telemetry...
tools/telemetry/telemetry/page_set_metadata.py:89: f.write(json.dumps(metadata,
indent=4))
On 2013/01/25 00:22:31, Dave Tu wrote:
> Just json.dump(metadata, indent=4)

Done.

https://codereview.chromium.org/11881051/diff/20031/tools/telemetry/telemetry...
File tools/telemetry/telemetry/record_wpr.py (right):

https://codereview.chromium.org/11881051/diff/20031/tools/telemetry/telemetry...
tools/telemetry/telemetry/record_wpr.py:84: # after the recording.
On 2013/01/25 00:22:31, Dave Tu wrote:
> Seems like the only thing you need before recording is setting a temporary wpr
> file. You can move all these metadata things after the recording. Then maybe
> wrap them all up into a function?

Done.

After we have recorded the pages, we still need to know which of them were run,
and update the metadata only for them; I added a function AddNewRecording for
doing that.

https://codereview.chromium.org/11881051/diff/20031/tools/telemetry/telemetry...
tools/telemetry/telemetry/record_wpr.py:91: wpr_files_not_needed =
ps.wpr_metadata.RemoveAbandonedWprFiles()
On 2013/01/25 10:34:06, nduca wrote:
> the interface on the wpr_metadata seems to mix side effects with retrieval.
Can
> you make it a bit more action-vs-access?
> 
> GetListOfAbandonedWprFiles() # side effect free
> metadata.DidRenameWprFile(oldName,newName)
> etc?

I have contradictory comments for this part:
nduca@: separate actions and retrieval better (i.e., move actions out of the
func)
dtu@: move more actions into the func

Internally, I split it into two: one func for finding out which files are
abandoned, and another one for removing them from the metadata and removing the
corresponding wpr files.

https://codereview.chromium.org/11881051/diff/20031/tools/telemetry/telemetry...
tools/telemetry/telemetry/record_wpr.py:120: if not results.page_failures:
On 2013/01/25 10:34:06, nduca wrote:
> what if someday 5 pages succeed and 1 fails. How hard will it be to make the
> code written today support better handling of that case? Its a good way to
test
> the cleanliness of the current patch. I think the architecutre isn't quite
ready
> for that, which means it needs refactoring --- not to actually implement what
I
> describe, but not require heavy refactoring.
> 
> I think the key is that we can write out the metadata at any time, and that as
> we successfully record a page, we update which archive it points at. And, if
we
> wanted, write the metadata at that point. As we do that, old archives will
> implicitly become orphaned and they can be deleted, or we can simply scavange
> them at the end [ps.wpr_metadata.unused_files()]

Here (in the new code) we know which pages have been ran, so we can only update
the recording for those pages. In the future we can do it so that this is done
even if the whole run has failed.

However, if the whole recording crashes, before we reach this point, then
nothing will be done. It would require more refactoring to actually write out
the metadata after recording each page...

https://codereview.chromium.org/11881051/diff/20031/tools/telemetry/telemetry...
tools/telemetry/telemetry/record_wpr.py:124: for wpr_file_path in
wpr_files_not_needed:
On 2013/01/25 10:34:06, nduca wrote:
> i am kind of worried about the ordering of the deletion calls here relative to
> the RemoveAbandonedWprFiles above. As designed, the metadata and filesystem
are
> in inconsistent states i think up until the end here. Can you figure out a way
> to make it continually consistent?

We have many moving parts; the new wpr to be added, N old wprs to delete, and
one metadata file. It cannot be "continually consistent".

I made it more robust though, to not fail e.g., if it can't delete one of the
old .wpr files. And changed the order of operations: it's always safe to move
the new .wpr file in place, that'll only be a new file on the file system, not
very annoying even if we fail after that. Then we write the metadata. And at
last, delete the files.

In theory we could do stuff like "roll back the changes if some file operations
fail", but...

[dtu, 2013-01-30 02:19:10]

https://chromiumcodereview.appspot.com/11881051/diff/37001/tools/telemetry/te...
File tools/telemetry/telemetry/page.py (right):

https://chromiumcodereview.appspot.com/11881051/diff/37001/tools/telemetry/te...
tools/telemetry/telemetry/page.py:25: self.archive_path = ''
None

https://chromiumcodereview.appspot.com/11881051/diff/37001/tools/telemetry/te...
File tools/telemetry/telemetry/page_set.py (right):

https://chromiumcodereview.appspot.com/11881051/diff/37001/tools/telemetry/te...
tools/telemetry/telemetry/page_set.py:81: def AddPage(self, page):
There are other places in telemetry that do pages.append. It might be only in
unit tests, though. Do you need to modify those as well?

https://chromiumcodereview.appspot.com/11881051/diff/37001/tools/telemetry/te...
File tools/telemetry/telemetry/record_wpr.py (right):

https://chromiumcodereview.appspot.com/11881051/diff/37001/tools/telemetry/te...
tools/telemetry/telemetry/record_wpr.py:111: target_wpr_file_path =
ps.wpr_archive_info.AddNewRecording(pages_run)
This method could take temp_target_wpr_file_path and do all of the below
implicitly. Then your PSArchiveInfo object would always be consistent with the
filesystem (from this point of view, assuming no errors).

[marja, 2013-01-30 12:26:22]

Thanks for comments again! dtu@, can you have another look?

https://codereview.chromium.org/11881051/diff/37001/tools/telemetry/telemetry...
File tools/telemetry/telemetry/page.py (right):

https://codereview.chromium.org/11881051/diff/37001/tools/telemetry/telemetry...
tools/telemetry/telemetry/page.py:25: self.archive_path = ''
On 2013/01/30 02:19:10, Dave Tu wrote:
> None

Done.

https://codereview.chromium.org/11881051/diff/37001/tools/telemetry/telemetry...
File tools/telemetry/telemetry/page_set.py (right):

https://codereview.chromium.org/11881051/diff/37001/tools/telemetry/telemetry...
tools/telemetry/telemetry/page_set.py:81: def AddPage(self, page):
On 2013/01/30 02:19:10, Dave Tu wrote:
> There are other places in telemetry that do pages.append. It might be only in
> unit tests, though. Do you need to modify those as well?

They pass as is, but I modified them to use AddPage, because it's cleaner.

https://codereview.chromium.org/11881051/diff/37001/tools/telemetry/telemetry...
File tools/telemetry/telemetry/record_wpr.py (right):

https://codereview.chromium.org/11881051/diff/37001/tools/telemetry/telemetry...
tools/telemetry/telemetry/record_wpr.py:111: target_wpr_file_path =
ps.wpr_archive_info.AddNewRecording(pages_run)
On 2013/01/30 02:19:10, Dave Tu wrote:
> This method could take temp_target_wpr_file_path and do all of the below
> implicitly. Then your PSArchiveInfo object would always be consistent with the
> filesystem (from this point of view, assuming no errors).

Done.

[dtu, 2013-01-30 23:28:32]

lgtm

[nduca, 2013-01-31 10:15:14]

this looks really good. the page.archive_path i'd like to have go away still,
PageTestResults.successes exists so that should help, and a few sundry nits.
I'll look again in the morning and we can probably get lg'd by end of your day.

https://codereview.chromium.org/11881051/diff/45001/tools/telemetry/telemetry...
File tools/telemetry/telemetry/page_runner.py (right):

https://codereview.chromium.org/11881051/diff/45001/tools/telemetry/telemetry...
tools/telemetry/telemetry/page_runner.py:70: def Run(self, options,
possible_browser, test, results, pages_run=None):
would love this edit to not be here

https://codereview.chromium.org/11881051/diff/45001/tools/telemetry/telemetry...
File tools/telemetry/telemetry/page_set.py (right):

https://codereview.chromium.org/11881051/diff/45001/tools/telemetry/telemetry...
tools/telemetry/telemetry/page_set.py:16: self.base_dir =
os.path.dirname(file_path)
minor nit: can we store self.file_path and make base_dir and file_name just
local vars when they're needed?

https://codereview.chromium.org/11881051/diff/45001/tools/telemetry/telemetry...
File tools/telemetry/telemetry/record_wpr.py (right):

https://codereview.chromium.org/11881051/diff/45001/tools/telemetry/telemetry...
tools/telemetry/telemetry/record_wpr.py:108: if not results.page_failures:
maybe we have a command line flag --update-archive-if-some-pages-fail that
writes anyawy, an then the failure case can sys.stderr.write('hey if youre
having trouble, you could use this flag ...')

https://codereview.chromium.org/11881051/diff/45001/tools/telemetry/telemetry...
tools/telemetry/telemetry/record_wpr.py:109: # Update the metadata for the pages
which were recorded.
is it sufficient o just say results.succeses here instead of pages run and then
we actually correctly handle pages timing out during failures?

[marja, 2013-01-31 14:44:58]

Thanks for comments again!

https://codereview.chromium.org/11881051/diff/45001/tools/telemetry/telemetry...
File tools/telemetry/telemetry/page_runner.py (right):

https://codereview.chromium.org/11881051/diff/45001/tools/telemetry/telemetry...
tools/telemetry/telemetry/page_runner.py:70: def Run(self, options,
possible_browser, test, results, pages_run=None):
On 2013/01/31 10:15:14, nduca wrote:
> would love this edit to not be here

pages_run? That's gone now. Or did you mean some other edit?

https://codereview.chromium.org/11881051/diff/45001/tools/telemetry/telemetry...
File tools/telemetry/telemetry/page_set.py (right):

https://codereview.chromium.org/11881051/diff/45001/tools/telemetry/telemetry...
tools/telemetry/telemetry/page_set.py:16: self.base_dir =
os.path.dirname(file_path)
On 2013/01/31 10:15:14, nduca wrote:
> minor nit: can we store self.file_path and make base_dir and file_name just
> local vars when they're needed?

Done.

https://codereview.chromium.org/11881051/diff/45001/tools/telemetry/telemetry...
File tools/telemetry/telemetry/record_wpr.py (right):

https://codereview.chromium.org/11881051/diff/45001/tools/telemetry/telemetry...
tools/telemetry/telemetry/record_wpr.py:108: if not results.page_failures:
On 2013/01/31 10:15:14, nduca wrote:
> maybe we have a command line flag --update-archive-if-some-pages-fail that
> writes anyawy, an then the failure case can sys.stderr.write('hey if youre
> having trouble, you could use this flag ...')

If the "update the archives for the succeeded pages" is the desired behavior, we
can achieve that easily, I modified this CL so it does so. (And added a warning
print above that "only successful pages have been updated".)

https://codereview.chromium.org/11881051/diff/45001/tools/telemetry/telemetry...
tools/telemetry/telemetry/record_wpr.py:109: # Update the metadata for the pages
which were recorded.
On 2013/01/31 10:15:14, nduca wrote:
> is it sufficient o just say results.succeses here instead of pages run and
then
> we actually correctly handle pages timing out during failures?

Done.

[marja, 2013-01-31 18:58:33]

https://codereview.chromium.org/11881051/diff/41002/tools/perf/page_sets/toug...
File tools/perf/page_sets/tough_canvas_cases.json (left):

https://codereview.chromium.org/11881051/diff/41002/tools/perf/page_sets/toug...
tools/perf/page_sets/tough_canvas_cases.json:3: "archive_path":
"../data/tough_canvas_cases.wpr",
Actually, this does not exist! Should I just remove this line instead?

[marja, 2013-01-31 19:03:12]

https://codereview.chromium.org/11881051/diff/41002/tools/perf/page_sets/toug...
File tools/perf/page_sets/tough_canvas_cases.json (left):

https://codereview.chromium.org/11881051/diff/41002/tools/perf/page_sets/toug...
tools/perf/page_sets/tough_canvas_cases.json:3: "archive_path":
"../data/tough_canvas_cases.wpr",
On 2013/01/31 18:58:33, marja wrote:
> Actually, this does not exist! Should I just remove this line instead?

Nothing, it was a bug, wiltzius fixed this.

[nduca, 2013-01-31 19:20:19]

lgtm, well done

[commit-bot: I haz the power, 2013-02-01 09:39:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/marja@chromium.org/11881051/46001

[commit-bot: I haz the power, 2013-02-01 10:04:33]

Change committed as 180117

[marja, 2013-02-01 12:15:01]

Fyi, this broke stuff because the test bots have python 2.6 and that doesn't
have OrderedDict.

It was unnecessary anyway, since OrderedDict is not (for some reason, didn't
investigate) serialized to json in alphabetical order of the keys.

[commit-bot: I haz the power, 2013-02-01 12:15:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/marja@chromium.org/11881051/28005

[commit-bot: I haz the power, 2013-02-01 12:16:18]

Change committed as 180133

[dtu, 2013-02-05 23:11:33]

On 2013/02/01 12:16:18, I haz the power (commit-bot) wrote:
> Change committed as 180133

The extra parameter to Page is making some perf unit tests fail.

[ RUN      ] SmoothnessBenchmarkUnitTest.testCalcResultsRealRenderStats
Traceback (most recent call last):
  File "/ssd/chrome/src/tools/perf/perf_tools/smoothness_benchmark_unittest.py",
line 69, in testCalcResultsRealRenderStats
    res.WillMeasurePage(page.Page('http://foo.com/'))
TypeError: __init__() takes at least 3 arguments (2 given)

[  FAILED  ] SmoothnessBenchmarkUnitTest.testCalcResultsRealRenderStats (0 ms)

[dtu, 2013-02-06 06:57:55]

On 2013/02/05 23:11:33, Dave Tu wrote:
> On 2013/02/01 12:16:18, I haz the power (commit-bot) wrote:
> > Change committed as 180133
> 
> The extra parameter to Page is making some perf unit tests fail.
> 
> [ RUN      ] SmoothnessBenchmarkUnitTest.testCalcResultsRealRenderStats
> Traceback (most recent call last):
>   File
"/ssd/chrome/src/tools/perf/perf_tools/smoothness_benchmark_unittest.py",
> line 69, in testCalcResultsRealRenderStats
>     res.WillMeasurePage(page.Page('http://foo.com/'))
> TypeError: __init__() takes at least 3 arguments (2 given)
> 
> [  FAILED  ] SmoothnessBenchmarkUnitTest.testCalcResultsRealRenderStats (0 ms)

Nevermind, I just TBR'd a quick fix to the test.
https://codereview.chromium.org/12207032/