Closed
Bug 364272
Opened 18 years ago
Closed 16 years ago
Ts performance regression on 12/15/06 from enabling places by default
Categories
(Firefox :: Bookmarks & History, defect, P2)
Tracking
()
RESOLVED
INCOMPLETE
People
(Reporter: moco, Unassigned)
References
()
Details
(Keywords: perf, regression)
Attachments
(1 file)
7.39 KB,
patch
|
Details | Diff | Splinter Review |
performance regression on 12/15/06 from enabling places by default
spun off from bug #355738.
robert wrote: "This landing regressed all performance metrics to some extent,
afaik."
details coming...
Reporter | ||
Comment 1•18 years ago
|
||
for regression specifics, see http://tinderbox.mozilla.org/showbuilds.cgi?tree=Firefox&hours=24&maxdate=1166264289&legend=0
look after my checkin at 2006/12/15 10:36:00
Specifically, there is a Ts regression of about 50 ms (details coming)
Reporter | ||
Comment 2•18 years ago
|
||
some links:
http://build-graphs.mozilla.org/graph/query.cgi?testname=startup&units=ms&tbox=bl-bldlnx01.office.mozilla.org_HEAD&autoscale=1&days=7&avg=1&showpoint=2006:12:15:21:11:01,2213
http://build-graphs.mozilla.org/graph/query.cgi?testname=startup&units=ms&tbox=xserve06.build.mozilla.org_Fx-Trunk&autoscale=1&days=7&avg=1&showpoint=2006:12:15:20:00:08,1459
http://build-graphs.mozilla.org/graph/query.cgi?testname=startup&units=ms&tbox=bm-xserve08.build.mozilla.org_Fx-Trunk&autoscale=1&days=7&avg=1&showpoint=2006:12:15:20:06:56,858
http://build-graphs.mozilla.org/graph/query.cgi?testname=startup&units=ms&tbox=bl-bldxp01_HEAD&autoscale=1&days=7&avg=1&showpoint=2006:12:15:20:37:15,1812
Reporter | ||
Comment 3•18 years ago
|
||
chatting with robert and dan in #places, the explanation for why Ts regressed was:
the profile we are using for Ts has a history.dat, so we are paying for the import (mork -> sqlite).
I need to find out more about the history.dat that we use with the profile we use with the Ts tests. (thunder, do you know how big it is?)
robert adds: "if there is no .sqlite you are paying for database creation too, even if there is nothing to import"
Comment 4•18 years ago
|
||
It would be nice to make decisions with data that approximates the way most of our users experience the product: a healthy history file on Microsoft Windows.
Marking depends on bug 364276.
Depends on: 364276
Reporter | ||
Updated•18 years ago
|
Reporter | ||
Updated•18 years ago
|
Summary: performance regression on 12/15/06 from enabling places by default → Ts performance regression on 12/15/06 from enabling places by default
Comment 5•18 years ago
|
||
Does this only affect the first startup (as a Mork database is converted to an SQLite database) or does it affect every startup?
Comment 6•18 years ago
|
||
rhelmer: does the tinderbox Ts test use a specific profile? or does it create a new one?
Status: NEW → ASSIGNED
Reporter | ||
Updated•18 years ago
|
Version: 2.0 Branch → Trunk
Reporter | ||
Comment 7•18 years ago
|
||
See bug #365967 and bug #365968 about Tp and Txul regressions on 12/15
Comment 8•18 years ago
|
||
(In reply to comment #6)
> rhelmer: does the tinderbox Ts test use a specific profile? or does it create a
> new one?
bl-bldlnx01 currently deletes and re-creates the profile between runs:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla%2Ftools%2Ftinderbox-configs%2Ffirefox%2Fwin32%2Ftinder-config.pl&rev=test_perf&cvsroot=%2Fcvsroot#56
Thunder was working on a perf test that would preserve history, just for this purpose, iirc.
Comment 9•18 years ago
|
||
Ok, looks like the Ts score is calculated by doing 10 runs and taking the best of the bunch. However, it does *not* clear the profile between runs. Which means that we're not taking a penalty for migration or db creation.
Ts source:
http://lxr.mozilla.org/mozilla/source/tools/tinderbox/build-seamonkey-util.pl#2960
Updated•18 years ago
|
Assignee: nobody → sspitzer
Status: ASSIGNED → NEW
Flags: blocking-firefox3+
Reporter | ||
Comment 10•18 years ago
|
||
sorry for the bug spam, re-assigning bugs back to default owner if I'm not working actively on them.
Assignee: sspitzer → nobody
Updated•18 years ago
|
Target Milestone: --- → Firefox 3 alpha6
Comment 11•17 years ago
|
||
This is blocking on bug 374532, which includes testing that'll give us a better idea of contemporary places Ts impact.
Depends on: 374532
Comment 12•17 years ago
|
||
retargeting bugs that don't meet the alpha release-blocker criteria at http://wiki.mozilla.org/Firefox3/Schedule.
Target Milestone: Firefox 3 alpha6 → Firefox 3 beta1
Updated•17 years ago
|
Target Milestone: Firefox 3 M7 → Firefox 3 M8
Updated•17 years ago
|
Target Milestone: Firefox 3 M8 → Firefox 3 M9
Comment 13•17 years ago
|
||
Bug 374532 shows a 2.1% increase in Ts between places and non-places on trunk as of 6/30/07.
Comment 14•17 years ago
|
||
Except when migrating on first installation, Places is initialized in nsBrowserGlue. It could move back to post-first-xul-window-load, which would mitigate it's impact on Ts.
Assignee: nobody → dietrich
Comment 15•17 years ago
|
||
Comment 16•17 years ago
|
||
(In reply to comment #15)
> Created an attachment (id=280505) [details]
> v1 - move places init to delayedStartup
>
I think that will still delay a usable browser window. Did the old history system init at this point?
Comment 17•17 years ago
|
||
(In reply to comment #16)
> (In reply to comment #15)
> > Created an attachment (id=280505) [details] [details]
> > v1 - move places init to delayedStartup
> >
>
> I think that will still delay a usable browser window. Did the old history
> system init at this point?
>
Yes, history on the branch starts up in delayedStartup.
Comment 18•17 years ago
|
||
moving out bugs that don't need to block b1
Target Milestone: Firefox 3 M9 → Firefox 3 M10
Updated•17 years ago
|
Priority: -- → P2
Updated•17 years ago
|
Target Milestone: Firefox 3 M10 → Firefox 3 M11
Comment 19•17 years ago
|
||
we need data on where places is slowing down startup. bug 406810 might help here.
Assignee: dietrich → ondrej
Depends on: 406810
Updated•17 years ago
|
Target Milestone: Firefox 3 beta3 → Firefox 3 beta4
Comment 20•17 years ago
|
||
The startup time seems to be dependent on number of bookmarks and independent of the history size (I got same results with 0 bookmarks and empty history versus 80MB history). Where can I get information about the bookmarks used for the test?
Someone please help me to see this 50ms regression. I cannot see it neither on charts nor in raw data.
Comment 21•17 years ago
|
||
I was running Ts on my computer - 100 executions and taking minimum time.
Places enabled: 1091ms
Places.xpt removed: 1049ms
This means that the difference is 42ms, that is 4% of the startup time is probably spent in places initialization. I will try to discover, where are those 42ms spent and if we can improve something.
Contribution of BrowserGlue.prototype._onProfileStartup._initPlaces()is not measurable, even with 100 executions. But this is expected as it does almost nothing.
Comment 22•17 years ago
|
||
Following data comes from analysis done for bug 406810 (thanks Michal!):
http://www.allpeers.com/download/mozilla/bug364272/nsNavHistory+Init.png
Suggested improvements:
1) Replace multiple calls to mozStorageConnection::TableExists with reading list of tables into memory in nsNavHistory::InitDb and passing this to other services initialized there (could save 0.04%).
2) Implement late preparing of SQL statements in nsNavHistory::InitStatements. The statements that are prepared in service initialization are not needed during startup. Statements should be initialized only when it is needed (before first use). This could save something like 0.3% of the startup time.
In both cases it is related to sqlite, it performs operations that probably did not have any equivalent before places introduction. I would suggest to implement this before doing any other analysis on this bug.
Status: NEW → ASSIGNED
Comment 23•17 years ago
|
||
Ondrej, if we remove the history/bookmarks bits from Fx2 startup, what's the overhead? Trying to figure out if we've fixed this over time, and if not what the current delta is.
Updated•17 years ago
|
Target Milestone: Firefox 3 beta4 → Firefox 3
Comment 24•17 years ago
|
||
I think this is hard to actually measure at this point. Bugs like this are kinda pointless, either we shouldn't take the hit at the time or we should back out. Its obviously too late for that now, we have what we have for this release.
(I do think the delta is much better than it used to be, but its still not optimal.)
Flags: blocking-firefox3+
Comment 25•17 years ago
|
||
I would prefer if bug 421513 becomes blocker. There we have a clear solution and it should improve both startup time and normal run (when we prepare all statements once instead of preparing them again and again).
Updated•16 years ago
|
Target Milestone: Firefox 3 → ---
Updated•16 years ago
|
Assignee: ondrej → nobody
Comment 26•16 years ago
|
||
I'm going through and marking old performance regression bugs as INCOMPLETE that are likely too old to be valid or get any traction on them. (Also, see comment 24 - this bug is useless at this point)
Please re-open if you have more information or can demonstrate the regression still exists.
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → INCOMPLETE
Comment 27•15 years ago
|
||
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
Component: Places → Bookmarks & History
QA Contact: places → bookmarks
You need to log in
before you can comment on or make changes to this bug.
Description
•