Last Comment Bug 644744 - Whitelist io that must happen before UI is shown, warn on everything else
: Whitelist io that must happen before UI is shown, warn on everything else
Status: RESOLVED FIXED
[Snappy:p2]
:
Product: Testing
Classification: Components
Component: Talos (show other bugs)
: unspecified
: x86 Linux
: -- normal with 2 votes (vote)
: ---
Assigned To: Aaron Klotz [:aklotz]
:
Mentors:
Depends on: 888369 609111 770317 863790 863809 864430 875914 878858 881771 900511 900545 900913
Blocks: 572459
  Show dependency treegraph
 
Reported: 2011-03-24 13:27 PDT by (dormant account)
Modified: 2013-08-28 13:12 PDT (History)
14 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Part 1: etlparser enhancements (11.52 KB, patch)
2013-03-15 14:23 PDT, Aaron Klotz [:aklotz]
jmaher: review+
Details | Diff | Review
Part 2: Changes to allow uploading of xperf mainthread I/O counters (4.68 KB, patch)
2013-03-19 09:37 PDT, Aaron Klotz [:aklotz]
jmaher: review-
Details | Diff | Review
Part 2: Changes to allow uploading of xperf mainthread I/O counters (rev 2) (4.39 KB, patch)
2013-03-25 11:59 PDT, Aaron Klotz [:aklotz]
jmaher: review+
Details | Diff | Review
Part 3: Enable the mainthread I/O counters in config (2.37 KB, patch)
2013-03-27 12:52 PDT, Aaron Klotz [:aklotz]
jmaher: review+
Details | Diff | Review

Description (dormant account) 2011-03-24 13:27:44 PDT
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] 2011-03-24 14:18:45 PDT
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) 2011-03-24 14:44:25 PDT
(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 2011-03-24 14:55:43 PDT
(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] 2011-03-28 10:46:43 PDT
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) 2011-03-28 10:48:11 PDT
(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) 2012-09-26 11:30:07 PDT
is this bug still useful?  We recently redid the xperf stuff to meet the needs of the perf team.
Comment 7 (dormant account) 2012-09-26 13:49:57 PDT
(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 Aaron Klotz [:aklotz] 2013-03-15 14:23:01 PDT
Created attachment 725601 [details] [diff] [review]
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) 2013-03-18 08:19:14 PDT
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 Aaron Klotz [:aklotz] 2013-03-19 09:22:16 PDT
(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 Aaron Klotz [:aklotz] 2013-03-19 09:37:55 PDT
Created attachment 726720 [details] [diff] [review]
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) 2013-03-20 11:15:06 PDT
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) 2013-03-20 11:23:12 PDT
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 Aaron Klotz [:aklotz] 2013-03-21 02:50:07 PDT
Part 1:
http://hg.mozilla.org/build/talos/rev/798fdabb0baa
Comment 15 Aaron Klotz [:aklotz] 2013-03-25 11:59:49 PDT
Created attachment 729117 [details] [diff] [review]
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) 2013-03-26 11:48:17 PDT
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 Aaron Klotz [:aklotz] 2013-03-27 12:52:35 PDT
Created attachment 730294 [details] [diff] [review]
Part 3: Enable the mainthread I/O counters in config

This patch simply turns on the new counters.
Comment 18 Aaron Klotz [:aklotz] 2013-03-27 12:53:03 PDT
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) 2013-03-27 13:01:34 PDT
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 Aaron Klotz [:aklotz] 2013-03-27 15:37:37 PDT
Part 3:
http://hg.mozilla.org/build/talos/rev/560806cfa208
Comment 21 Aaron Klotz [:aklotz] 2013-03-27 16:01:22 PDT
Deployment is in bug 855507.
Comment 22 Aaron Klotz [:aklotz] 2013-08-28 13:12:51 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.