399476 - bug #387996 caused Txul and Ts regressions

Status: VERIFIED FIXED

(Firefox :: Bookmarks & History, defect, P2)

Description

[(not reading, please use [e-mail] instead)]

from irc:

<sayrer> sspitzer regressed Ts last night
<sayrer> sspitzer, yeah Txul and Ts on all platforms :/

<mfinkle> sspitzerMsgMe: not even a little bit :)
<mfinkle> 956ms -> 972ms
<mfinkle> on linux
<mfinkle> Ts

<sspitzerMsgMe> part of my patch does some work in browser.js one time (creating the "Places" folder), on delayed startup.  but if we start with a new profile, or a profile-from-before-my-fix, we'd pay that price every time.
<sspitzerMsgMe> sayrer / mfinkle:  let me take this to a bug, I'll cc you guys.

my plan would be to make the default profile have the places folder and the queries (so the hidden browser.places.createdDefaultQueries pref would be set to true) and then see the Ts / Txul numbers.

otherwise, we're measuring code that that the user runs once.

Comment 1

[(not reading, please use [e-mail] instead)]

rob, can you help me with changing the default profile for tinderbox?

Comment 2

[Benjamin Smedberg]

I don't think the profile issue is the problem: the Ts test does N startups (10?) and discards the slowest one. And that doesn't explain the Txul times anyway.

Comment 3

[(not reading, please use [e-mail] instead)]

> I don't think the profile issue is the problem

does it use a new profile each time or do start ups #2-#10 use the profile created from start up #1?

For Txul, I'm creating a new folder on the personal toolbar and inserting 6 bookmarked queries into it, which could affect Txul.

But even without creating the new folder, we'd still have a new folder on the personal toolbar that wasn't there before, which could affect Txul.

Comment 4

[(not reading, please use [e-mail] instead)]

For the Ts regression:

looking at StartupPerformanceTest in http://lxr.mozilla.org/mozilla/source/tools/tinderbox/build-seamonkey-util.pl#3211, it appears that we kill the process after each run to figure out the best start up time (out of 10).

in my code for bug #387996, I'm setting a hidden pref to avoid re-adding the special "Places" folder (with the pre-defined queries) on the personal toolbar folder.

since we aren't flushing prefs to disk via an explicit call to savePrefFile(), what might be going on is that we are paying the "one time only" cost each time.

Comment 5

[(not reading, please use [e-mail] instead)]

I've disabled the feature in bug #387996 by setting the default pref to be true in browser/app/profile/firefox.js:

+// disable adding the "Places" folder with pre-defined queries
+pref("browser.places.createdDefaultQueries", true);

before bug #387996:
Txul:243ms
Ts:956ms

after bug #387996:
Txul:256ms
Ts:972ms

after disabling bug #387996 via a pref:
Txul:239ms
Ts:957ms

For the Txul test (./firefox.exe -chrome "file:///c:/builds/trunk/mozilla/xpfe/test/winopen.xul"), if we start out with a new profile at the start of opening 10 windows, the first one will pay the "one time only cost" but the rest would then pay for having an additional item in the personal toolbar.

The Txul number is an average of 10 runs, that might explain the 16 ms bump.

Comment 6

[(not reading, please use [e-mail] instead)]

> The Txul number is an average of 10 runs, that might explain the 16 ms bump.

correction, looking at mozilla/xpfe/test/winopen.js and mozilla/ xpfe/test/winopen.xul, Txul is the average of the last 9 of 10 runs, excluding the first one.

Investigating if the new "Places" folder on the personal toolbar (created on the first run) explain a 16 ms bump in Txul.

Comment 7

[(not reading, please use [e-mail] instead)]

from my local tests (on Mac) this additional folder on the personal toolbar is costing 10 ms.

as for Ts, I'd to change the default profile so that we aren't paying the "first run cost" on each run.

Alternatively, I could use a "kill" safe way (instead of prefs) to determine if the queries were created.  I could call savePrefFile() after setting the pref.

Comment 8

[Dietrich Ayala (:dietrich)]

(In reply to comment #7)
> from my local tests (on Mac) this additional folder on the personal toolbar is
> costing 10 ms.
> 

Creating the folder itself? or creating and filling it?

Either way, that number is crazy given that you're really only creating a folder and adding 6 bookmarks to it.

Comment 9

[(not reading, please use [e-mail] instead)]

> Creating the folder itself? or creating and filling it?

To clarify, I'm not measuring the time to create or fill it, I'm measuring the impact having the folder has on Txul.

Comment 10

[(not reading, please use [e-mail] instead)]

Asaf, do you plan on working on Txul, Ts, or both?

Comment 11

[Asaf Romano (gone)]

Attachment: some improvements

* batch(!)
 * initialize the toolbar only after creating the queries

Comment 12

[Asaf Romano (gone)]

Attachment: initialize places-popups lazily

Comment 13

[(not reading, please use [e-mail] instead)]

Comment on attachment 284646 [details] [diff] [review]
initialize places-popups lazily

r=sspitzer.

from my Txul tests on my mac, this will lower our number.

Locally, by %1 (5 ms), so I'm curious to see what happens on tbox.

Comment 14

[(not reading, please use [e-mail] instead)]

Comment on attachment 284645 [details] [diff] [review]
some improvements

r=sspitzer, sorry for the delay Asaf.

on my mac, these changes save me 12 ms.

Comment 15

[Asaf Romano (gone)]

mozilla/browser/base/content/browser.js 1.869
mozilla/browser/components/places/content/menu.xml 1.87

Comment 16

[(not reading, please use [e-mail] instead)]

the call to savePrefFile() might be coming (for bug #399460, pending review).

if so, that should lower Ts.

Comment 17

[(not reading, please use [e-mail] instead)]

mano has landed his patches, and with his patches I'm not seeing the leaks (see
bug #399418 ), and I've landed the patch for bug #399460 (so Ts should be
better), so let's re-enable this feature on tinderbox and get some new Txul /
Ts numbers.

Comment 18

[(not reading, please use [e-mail] instead)]

after re-enabling, I'm seeing a 10ms cost to Txul and a 10ms cost to Ts

MacOSX Darwin 8.8.4 bm-xserve08 Dep Universal Nightly
before:
Txul:143ms
Ts:865ms

after:
Txul:153ms
Ts:874ms

Linux bl-bldlnx03 Dep fx-linux-tbox perf test
before:
Txul:241ms
Ts:956ms

after:
Txul:248ms
Ts:966ms

Comment 19

[Mike Connor [:mconnor]]

Looks like Asaf's views patch got most of the Txul hit back by fixing our views to not be insane, which is where we were hurting from the added bits to the default profile.  Ts went down some, but not completely.

Without the Places folder, does the patch regress anything, or are we just screwing with the baseline by adding more stuff to the profile?  I'm going to move this out until we can answer this more clearly.

Comment 20

[Robert Sayre]

One question to ask is whether we need 6 views in that menu.

Comment 21

[Asaf Romano (gone)]

That menu isn't even built until you open it...

Comment 22

[Asaf Romano (gone)]

The rest of the Ts regression was fixed in bug 401753.

Comment 23

[(not reading, please use [e-mail] instead)]

> The rest of the Ts regression was fixed in bug 401753.

Cool, nice work!

But, if I remember correctly, we still pay a Txul price for having an additional folder on the personal toolbar.

this bug covered both Ts and Txul.  Should we spin up a new one about just Txul?

Comment 24

[Asaf Romano (gone)]

All/most of the Txul part was fixed by bug 399729 afaict.

Comment 25

[(not reading, please use [e-mail] instead)]

> All/most of the Txul part was fixed by bug 399729 afaict.

short version:  ok

long version:  I think we need a new bug about the toolbar and Txul

details:

When I last tried on my mac, I was able to consistently measure that every item on the personal toolbar affected Txul.  (And, Txul impacts Ts.)

small fluctuations in Ts is a wild goose that I prefer not to chase.  since we are calling savePrefFile(), and Ts takes the best of <n> times, the "real" Ts cost of creating this folder and these queries is not being measured.)

I think it would be worth re-measuring, so see what the current cost is.

it might be linear (per item), or worse.  (New profiles have 3 items, but if "real" users have more, then it could be worse, and worth profiling / investigating / fixing to improve Ts / Txul.)

from my notes, I also think we might be calling updateChevron() twice.

Note, We're going have to pay some Ts / Txul price for this "places" folder.

I'll log a perf bug on these issues.

Comment 26

[Tracy Walker [:tracy]]

plenty of bake time

Comment 27

[Gervase Markham [:gerv]]

Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h".

In Thunderbird 3.0b, you do that as follows:
Tools | Message Filters
Make sure the correct account is selected. Click "New"
Conditions: Body   contains   places-to-b-and-h
Change the action to "Delete Message".
Select "Manually Run" from the dropdown at the top.
Click OK.

Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter.

Gerv