Closed Bug 907559 Opened 8 years ago Closed 8 years ago

Update the PGO profile to open and close tabs between test pages

Categories

(Firefox Build System :: General, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla26

People

(Reporter: jaws, Assigned: jaws)

Details

Attachments

(1 file)

Attached patch PatchSplinter Review
The current PGO profile opens a new tab and then loads various test pages within that tab. After completing, it closes the window containing the new tab.

It is well understood that users of Firefox use more than one tab, and so it should make more sense that the profile include more exercising of the tab open and tab close code paths.

This is a baseline try push for comparison:
https://tbpl.mozilla.org/?tree=Try&rev=225be8b41ffe

This is a try push that opens and closes a tab between each test page:
https://tbpl.mozilla.org/?tree=Try&rev=9b11c90630c5

This is a link to the compare-talos output:
http://compare-talos.mattn.ca/?oldRevs=225be8b41ffe&newRev=9b11c90630c5&submit=true

I have attached the changes to the PGO profile to this bug.

Because TART (Tab Animation Regression Test) is not on m-c yet, these results do not show how TART would be affected.
Assignee: nobody → jaws
Status: NEW → ASSIGNED
I'm not convinced this would change much to the profile. That's only going to trigger more js code, which won't really translate to significant changes to the profile, since js is mostly JITed, and for what's not JITed, we should have a pretty good coverage already.
This ended up giving good performance wins on dromaeo_dom (-1.5% to -4.5%). Still need to check TART times.
(In reply to Jared Wein [:jaws] from comment #2)
> This ended up giving good performance wins on dromaeo_dom (-1.5% to -4.5%).

That sounds very unexpected. You're not changing anything that should affect that.
(In reply to Mike Hommey [:glandium] from comment #3)
> (In reply to Jared Wein [:jaws] from comment #2)
> > This ended up giving good performance wins on dromaeo_dom (-1.5% to -4.5%).
> 
> That sounds very unexpected. You're not changing anything that should affect
> that.

Opening and closing the tab will execute more DOM code, so it isn't too crazy for the perf of DOM related code to improve.
(In reply to Jared Wein [:jaws] from comment #4)
> Opening and closing the tab will execute more DOM code, so it isn't too
> crazy for the perf of DOM related code to improve.

I'd expect the tested pages to use more DOM code than opening and closing tabs. But maybe my expectations are wrong. If there isn't much DOM code fiddling in the tested pages, then that should be addressed directly instead, imho. That is, it's nice if your patch is improving dromaeo dom, but it shouldn't, and I don't think it's right to rely on this patch to improve dromaeo dom results, as it's only a side effect.
To be clear, the goal of this bug is to improve the performance of the TART test, of which I don't have numbers for yet. It is however important to note that it doesn't regress performance of other tests (and in fact improves the performance of one of them!) :)
If it doesn't regress any benchmarks then I have no opposition to changing the profile. This is all pretty much a black box anyway.
These are the numbers from TART:
TART.simple-open-DPI1.raw:
-2.81%

TART.simple-close-DPI1.raw:
+12.96%

TART.icon-open-DPI1.raw:
-11.68%

TART.icon-close-DPI1.raw:
-26.58%

TART.icon-open-DPI2.raw:
+5.22%

TART.icon-close-DPI2.raw:
-12.8%

TART.iconFade-close-DPI2.raw:
+6.25%

TART.iconFade-open-DPI1.raw:
+1.17%

TART.newtab-open-preload-no.raw:
+6.89%

TART.newtab-open-preload-yes.raw:
+12.30%

Negative is an improvement, positive is a regression.
Attachment #793323 - Flags: review?(ted)
Attachment #793323 - Flags: review?(ted) → review+
Summary: Investigate altering PGO profile to open and close tabs → Update the PGO profile to open and close tabs between test pages
https://hg.mozilla.org/mozilla-central/rev/984f2350c97f
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 26
(In reply to Jared Wein [:jaws] from comment #8)
> These are the numbers from TART:
> TART.simple-open-DPI1.raw:
> -2.81%
> ...

Since the .raw values are the discrete intervals of which a single animation case is composed (and are not reported to talos/graphserver/datazilla but only appear at the logs), I'm guessing that the changes you're posting refer to one of the other values of each animation?

For clarification:
"simple" is opening/closing about:blank.
"icon" is opening/closing blank page with (translucent) favicon and a long title.
"newtab" opens about:newtab with/without preload.
"iconFade" opens the "icon" tab and then measures fadeout/fadein (so no overhead of tab open/close).

DPIX is the DPI scaling with which the animation was executed.

The intervals for the entire animation are collected (and appear as .raw at the logs), and from those values, 3 test results are reported per animation:
[test].half : average interval over the 2nd half (duration-wise) of the animation.
[test].all  : average of all the intervals of this animation.
[test].error: absolute difference between the designated duration to the actual duration of the animation.
Sorry, when I used the *.raw name, I had meant to use the *.all name. I used the *.all values when calculating those numbers, not the *.raw. Thanks for catching that.
Component: Build Config → General
Product: Firefox → Firefox Build System
Target Milestone: Firefox 26 → mozilla26
You need to log in before you can comment on or make changes to this bug.