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)

x86
All
defect

Tracking

()

RESOLVED INCOMPLETE

People

(Reporter: moco, Unassigned)

References

()

Details

(Keywords: perf, regression)

Attachments

(1 file)

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...
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)
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"




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
Blocks: 355738
No longer depends on: 355738
Summary: performance regression on 12/15/06 from enabling places by default → Ts performance regression on 12/15/06 from enabling places by default
Does this only affect the first startup (as a Mork database is converted to an SQLite database) or does it affect every startup?
rhelmer: does the tinderbox Ts test use a specific profile? or does it create a new one?
Status: NEW → ASSIGNED
Version: 2.0 Branch → Trunk
See bug #365967 and bug #365968 about Tp and Txul regressions on 12/15
(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.
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
Assignee: nobody → sspitzer
Status: ASSIGNED → NEW
Flags: blocking-firefox3+
sorry for the bug spam, re-assigning bugs back to default owner if I'm not working actively on them.
Assignee: sspitzer → nobody
Target Milestone: --- → Firefox 3 alpha6
This is blocking on bug 374532, which includes testing that'll give us a better idea of contemporary places Ts impact.
Depends on: 374532
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
Target Milestone: Firefox 3 M7 → Firefox 3 M8
Target Milestone: Firefox 3 M8 → Firefox 3 M9
Bug 374532 shows a 2.1% increase in Ts between places and non-places on trunk as of 6/30/07.
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
(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?
(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.
moving out bugs that don't need to block b1
Target Milestone: Firefox 3 M9 → Firefox 3 M10
Priority: -- → P2
Keywords: perf
Target Milestone: Firefox 3 M10 → Firefox 3 M11
we need data on where places is slowing down startup. bug 406810 might help here.
Assignee: dietrich → ondrej
Depends on: 406810
Target Milestone: Firefox 3 beta3 → Firefox 3 beta4
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.
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.
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
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.
Target Milestone: Firefox 3 beta4 → Firefox 3
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+
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).
Target Milestone: Firefox 3 → ---
Assignee: ondrej → nobody
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
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.

Attachment

General

Created:
Updated:
Size: