644744 - Whitelist io that must happen before UI is shown, warn on everything else

Status: RESOLVED FIXED

(Testing :: Talos, defect)

Description

[(dormant account)]

Currently we do a lot of IO(eg sqlite) before UI is shown. We should be able to move most of that IO to happen after UI is drawn. In the meantime we should automatically gather a list of files the are accessed before UI is up to keep track of progress/regressions.

Comment 1

[Emanuel Hoogeveen [:ehoogeveen]]

If the IO happens after the UI is shown, can it slow down or stall the UI? I don't mind waiting a bit for the UI to show up, but having it stall after it appears ready is annoying. This already happens to some extent and is very noticeable when running Firefox from a pen drive on my university's computers, and I'm wondering if this bug will make it worse (they also have a virus scanner running that likes to scan everything the pen drive reads and writes, not much can be done about that).

Comment 2

[(dormant account)]

(In reply to comment #1)
> If the IO happens after the UI is shown, can it slow down or stall the UI? I
> don't mind waiting a bit for the UI to show up, but having it stall after it
> appears ready is annoying. This already happens to some extent and is very
> noticeable when running Firefox from a pen drive on my university's computers,
> and I'm wondering if this bug will make it worse (they also have a virus
> scanner running that likes to scan everything the pen drive reads and writes,
> not much can be done about that).

Responsiveness after UI is shown is a different issue. Our goal is to never have the UI blocked by IO. Having a hidden UI blocked on IO is no better than having a visible UI blocked by IO :)

Comment 3

[Shawn Wilsher :sdwilsh]

(In reply to comment #2)
> Responsiveness after UI is shown is a different issue. Our goal is to never
> have the UI blocked by IO. Having a hidden UI blocked on IO is no better than
> having a visible UI blocked by IO :)
Agreed.  These two things are orthogonal.

Comment 4

[alice nodelman [:alice] [:anode]]

Is this a talos bug?  It sounds like you are changing the behavior of the browser and not the testing harness...

Comment 5

[(dormant account)]

(In reply to comment #4)
> Is this a talos bug?  It sounds like you are changing the behavior of the
> browser and not the testing harness...

This is an extension of bug 609111 to do some post-processing on the xperf data. I think that qualifies it as a talos bug.

Comment 6

[Joel Maher ( :jmaher ) (UTC -8)]

is this bug still useful?  We recently redid the xperf stuff to meet the needs of the perf team.

Comment 7

[(dormant account)]

(In reply to Joel Maher (:jmaher) from comment #6)
> is this bug still useful?  We recently redid the xperf stuff to meet the
> needs of the perf team.

Yes we should do this.

Comment 8

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Part 1: etlparser enhancements

This patch modifies several things:
- Bug fixes for the state machine used for parsing the header section of the xperf csv;
- Modification of per-file I/O stats to include thread ID and stage;
- Facilities to specify a whitelist file in options and to load that whitelist;
- Adds filtering to per-file I/O stats so that output can be restricted to startup, main thread I/O whose files are not whitelisted. The filtering may be disabled via options.

Comment 9

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 725601 [details] [diff] [review]
Part 1: etlparser enhancements

Review of attachment 725601 [details] [diff] [review]:
-----------------------------------------------------------------

overall I like this patch, but there are a few questions I have which might be a simple answer, but enough for me to r- for now.

::: talos/xtalos/etlparser.py
@@ +277,5 @@
>      outFile.write(header + "\n")
>  
> +    # Filter out stages, threads, and whitelisted files that we're not interested in
> +    filekeys = filter(lambda x: (all_stages or x[2] == stages[0]) and 
> +                                (all_threads or x[1].endswith("(main)")) and

we default to the main thread?

@@ +290,5 @@
> +                                                   row[2],
> +                                                   files[row]['DiskReadCount'],
> +                                                   files[row]['DiskReadBytes'],
> +                                                   files[row]['DiskWriteCount'],
> +                                                   files[row]['DiskWriteBytes'])

do we really have files[row]?  before it was a filename, now it is a filekey which is an array?  I would think by earlier comments this should be:
files[row[0]]['Disk...']

Comment 10

[(No longer employed by Mozilla) Aaron Klotz]

(In reply to Joel Maher (:jmaher) from comment #9)
> Comment on attachment 725601 [details] [diff] [review]
> we default to the main thread?
Right now we're mostly interested in main thread I/O, so yes, as written it defaults to main thread unless all_threads has been specified in the config.

> do we really have files[row]?  before it was a filename, now it is a filekey
> which is an array?  I would think by earlier comments this should be:
> files[row[0]]['Disk...']
files is now keyed on a (filename, tid, stage) tuple. The filtering is done on files.iterkeys(), so filekeys is a list of such tuples that passed the filter.

Comment 11

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Part 2: Changes to allow uploading of xperf mainthread I/O counters

I added a bit of a hack to output.py so that the counters could be associated with filenames. I wasn't too sure if that was okay or not. It omits counters that are zero.

Sample output from these changes:

JSON output (ellipses added to condense some things):

[
  {
  ...
    "results_aux": {
      "mainthread_writecount": [
        [
          4, 
          "C:\\$LogFile"
        ], 
        [
          34, 
          "C:\\Users\\foo\\AppData\\Roaming\\Mozilla\\Firefox\\Profiles\\c040kp90.default\\compatibility.ini"
        ], 
        [
          16, 
          "C:\\Users\\foo\\AppData\\Roaming\\Mozilla\\Firefox\\Profiles\\c040kp90.default\\prefs-1.js"
        ]
      ]
    }, 
    ...
  }
]

Graphserver values:

START
VALUES
qm-pxp01,tp4m_mainthread_writecount_paint,,64a392780ed5,20130315110258,1363621376
0,4.00,C:\$LogFile
1,34.00,C:\Users\aklotz\AppData\Roaming\Mozilla\Firefox\Profiles\c040kp90.default\compatibility.ini
2,16.00,C:\Users\aklotz\AppData\Roaming\Mozilla\Firefox\Profiles\c040kp90.default\prefs-1.js
END

Comment 12

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 725601 [details] [diff] [review]
Part 1: etlparser enhancements

Review of attachment 725601 [details] [diff] [review]:
-----------------------------------------------------------------

switching to r+ after response to my comments.

Comment 13

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 726720 [details] [diff] [review]
Part 2: Changes to allow uploading of xperf mainthread I/O counters

Review of attachment 726720 [details] [diff] [review]:
-----------------------------------------------------------------

this is r- because it will fail when it comes to graph server.  We should not post at all to graph server and only datazilla, but we would still need to ensure this data plays well with datazilla.  Graph server requires pre-defined keys/values for all the stuff we are uploading.

Comment 14

[(No longer employed by Mozilla) Aaron Klotz]

Part 1:
http://hg.mozilla.org/build/talos/rev/798fdabb0baa

Comment 15

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Part 2: Changes to allow uploading of xperf mainthread I/O counters (rev 2)

This version modifies the graphserver output to skip any counters that won't work, i.e. the mainthreadio* stuff.

Comment 16

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 729117 [details] [diff] [review]
Part 2: Changes to allow uploading of xperf mainthread I/O counters (rev 2)

Review of attachment 729117 [details] [diff] [review]:
-----------------------------------------------------------------

this looks great.  Please ensure this works on try server and I would be happy to help land/deploy.

Comment 17

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Part 3: Enable the mainthread I/O counters in config

This patch simply turns on the new counters.

Comment 18

[(No longer employed by Mozilla) Aaron Klotz]

Checkin for part 2:
http://hg.mozilla.org/build/talos/rev/04374062fabb

Here's a try run to test everything out, which also included patch 3:
https://tbpl.mozilla.org/?tree=Try&rev=e21c49e58b26

Other than patch 3, what else (if anything) needs to be done to deploy?

Comment 19

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 730294 [details] [diff] [review]
Part 3: Enable the mainthread I/O counters in config

Review of attachment 730294 [details] [diff] [review]:
-----------------------------------------------------------------

this is good.  I looked at the try server, there were no side effects, but I didn't see the data specifically that we are turning on here.  Next steps  are to land this patch on talos, get releng to upload talos.zip and then land a change to talos.json referencing the new talos.zip (please reference bug 853868)

Comment 20

[(No longer employed by Mozilla) Aaron Klotz]

Part 3:
http://hg.mozilla.org/build/talos/rev/560806cfa208

Comment 21

[(No longer employed by Mozilla) Aaron Klotz]

Deployment is in bug 855507.

Comment 22

[(No longer employed by Mozilla) Aaron Klotz]

We now have Oranges on TBPL when xperf picks up new main thread I/O that's not on the whitelist. I think we can resolve this now and put further enhancements into new bugs.