The default bug view has changed. See this FAQ.

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

RESOLVED FIXED

Status

Testing
Talos
RESOLVED FIXED
6 years ago
4 years ago

People

(Reporter: (dormant account), Assigned: aklotz)

Tracking

(Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [Snappy:p2])

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

6 years ago
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.
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).
(Reporter)

Comment 2

6 years ago
(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 :)
(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.
Is this a talos bug?  It sounds like you are changing the behavior of the browser and not the testing harness...
(Reporter)

Comment 5

6 years ago
(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.
Whiteboard: [Snappy]
(Reporter)

Updated

5 years ago
Whiteboard: [Snappy] → [Snappy:p2]
is this bug still useful?  We recently redid the xperf stuff to meet the needs of the perf team.
(Reporter)

Comment 7

5 years ago
(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.
Depends on: 770317
(Assignee)

Updated

4 years ago
Assignee: nobody → aklotz
(Assignee)

Comment 8

4 years ago
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.
Attachment #725601 - Flags: review?(jmaher)
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...']
Attachment #725601 - Flags: review?(jmaher) → review-
(Assignee)

Comment 10

4 years ago
(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.
(Assignee)

Comment 11

4 years ago
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
Attachment #726720 - Flags: review?(jmaher)

Updated

4 years ago
Attachment #726720 - Attachment is patch: true
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.
Attachment #725601 - Flags: review- → review+
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.
Attachment #726720 - Flags: review?(jmaher) → review-
(Assignee)

Comment 14

4 years ago
Part 1:
http://hg.mozilla.org/build/talos/rev/798fdabb0baa
(Assignee)

Comment 15

4 years ago
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.
Attachment #726720 - Attachment is obsolete: true
Attachment #729117 - Flags: review?(jmaher)
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.
Attachment #729117 - Flags: review?(jmaher) → review+
(Assignee)

Comment 17

4 years ago
Created attachment 730294 [details] [diff] [review]
Part 3: Enable the mainthread I/O counters in config

This patch simply turns on the new counters.
Attachment #730294 - Flags: review?(jmaher)
(Assignee)

Comment 18

4 years ago
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 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)
Attachment #730294 - Flags: review?(jmaher) → review+
(Assignee)

Comment 20

4 years ago
Part 3:
http://hg.mozilla.org/build/talos/rev/560806cfa208
(Assignee)

Comment 21

4 years ago
Deployment is in bug 855507.
(Assignee)

Updated

4 years ago
Depends on: 863809, 864430
(Assignee)

Updated

4 years ago
Depends on: 863790
(Assignee)

Comment 22

4 years ago
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.
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.