Delay the database connection in the history service as far as possible

ASSIGNED
Assigned to

Status

()

Toolkit
Places
P1
normal
ASSIGNED
15 days ago
2 days ago

People

(Reporter: mak, Assigned: mak)

Tracking

(Blocks: 1 bug, {perf})

55 Branch
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [fxsearch])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

15 days ago
Things like this:
http://searchfox.org/mozilla-central/rev/a798ee4fc323f9387b7576dbed177859d29d09b7/browser/base/content/sanitize.js#
let shutdownClient = Cc["@mozilla.org/browser/nav-history-service;1"]
                             .getService(Ci.nsPIPlacesDatabase)
                             .shutdownClient
                             .jsclient;

don't need a database, but we still open the connection. Adding a shutdown blocker may be a common thing to do on initialization of a component.

We can probably wrap mDB in a getter and move the database init to happen lazily into it. We just need to be careful on shutdown (we don't want to create a db just to shut it down).
(Assignee)

Updated

15 days ago
Whiteboard: [fxsearch]
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Created attachment 8876863 [details] [diff] [review]
Test

Attaching here the test changes I had initially made for bug 1371710 in case it is helpful.
(Assignee)

Updated

11 days ago
Depends on: 1371945
(Assignee)

Comment 4

9 days ago
(In reply to Florian Quèze [:florian] [:flo] from comment #3)
> Created attachment 8876863 [details] [diff] [review]
> Test
> 
> Attaching here the test changes I had initially made for bug 1371710 in case
> it is helpful.

Is it enough to just run the test locally? I remember you said there was an unexpected failure on Windows, but if I run it on Windows I don't see it. Maybe I should do a run on Try.
Flags: needinfo?(florian)
(Assignee)

Comment 5

9 days ago
I also don't see any failure without my patch.
As well as I don't see a interesting benefit on ts_paint.
I see that we delay database initialization to after the window has been painted though.

The patch will be good regardless since it simplifies some of the startup path and removes the need for some old tricks to avoid paying Places init costs... but I don't expect to see a relevant gain on our tests harness, especially after bug 1371710 already removed the most obvious culprits.
(In reply to Marco Bonardo [::mak] from comment #4)
> (In reply to Florian Quèze [:florian] [:flo] from comment #3)
> > Created attachment 8876863 [details] [diff] [review]
> > Test
> > 
> > Attaching here the test changes I had initially made for bug 1371710 in case
> > it is helpful.
> 
> Is it enough to just run the test locally? I remember you said there was an
> unexpected failure on Windows, but if I run it on Windows I don't see it.
> Maybe I should do a run on Try.

I would push to try to be sure, just the performance folder is enough, eg.
 try: -b do -p linux64,macosx64,win32,win64 -u mochitest-bc,mochitest-e10s-bc -t none --try-test-paths browser-chrome:browser/base/content/test/performance

(In reply to Marco Bonardo [::mak] from comment #5)
> I also don't see any failure without my patch.
> As well as I don't see a interesting benefit on ts_paint.

ts_paint doesn't measure first paint of the browser window, but first paint of a webpage. We are hopefully fixing this in bug 1369417.
Flags: needinfo?(florian)
(Assignee)

Comment 7

9 days ago
This will also show a regression in "tp5n main_normal_fileio opt e10s" and a pretty-much-identical win in "tp5n main_startup_fileio opt e10s"

That is probably normal considering we are moving the connection opening later in time (from my last measurement it's the Star button in the UI now that inits Places).

I still have one X-File to solve before asking for review, and will run the perf tests on try (I must re-run all the tests regardless once I figure this out).
(Assignee)

Updated

9 days ago
No longer depends on: 1371945
Comment hidden (mozreview-request)
(Assignee)

Comment 9

5 days ago
Comment on attachment 8876863 [details] [diff] [review]
Test

The startup test changes landed already, so this is no more needed.
Attachment #8876863 - Attachment is obsolete: true
Comment hidden (mozreview-request)
(Assignee)

Comment 11

5 days ago
Sorry, this is not a trivial patch to review, feel free to ask any questions or do multi-pass.
Unfortunately it was not trivial to split it because every changed ended up creating a domino effect on something else.
This is a Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=00f34dbc27b8574edb9412b640cbd010f2ac8850
(Assignee)

Updated

2 days ago
Keywords: perf
You need to log in before you can comment on or make changes to this bug.