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

RESOLVED FIXED in Firefox 56

Status

()

Toolkit
Places
P1
normal
RESOLVED FIXED
2 months ago
22 days ago

People

(Reporter: mak, Assigned: mak)

Tracking

(Blocks: 2 bugs, {perf})

55 Branch
mozilla56
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox56 fixed)

Details

(Whiteboard: [fxsearch])

MozReview Requests

()

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

Attachments

(1 attachment, 1 obsolete attachment)

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

2 months 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

2 months ago
Depends on: 1371945
(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)
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)
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

2 months ago
No longer depends on: 1371945
Comment hidden (mozreview-request)
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)
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 months ago
Keywords: perf
Blocks: 1377600
Blocks: 1377599
Blocks: 1377598

Comment 12

a month ago
Sorry for not reviewing this yet, Marco.  I'll try to finish it ASAP.

Comment 13

a month ago
mozreview-review
Comment on attachment 8876207 [details]
Bug 1371677 - Delay the database connection in the history service as far as possible.

https://reviewboard.mozilla.org/r/147652/#review160820

LGTM...

::: toolkit/components/places/Database.cpp:572
(Diff revision 4)
> +  if (mMainConn ||
> +      mDatabaseStatus == nsINavHistoryService::DATABASE_STATUS_LOCKED) {
> +    return NS_OK;
> +  }
> +  // Don't try to create a database too late.
> +  if (IsShutdownStarted())

Nit: braces?
Attachment #8876207 - Flags: review?(adw) → review+
Comment hidden (mozreview-request)

Comment 15

a month ago
Pushed by mak77@bonardo.net:
https://hg.mozilla.org/integration/autoland/rev/43bfc32f2623
Delay the database connection in the history service as far as possible. r=adw

Comment 16

a month ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/43bfc32f2623
Status: ASSIGNED → RESOLVED
Last Resolved: a month ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
This caused some build regressions. Only if you have the time, could you take a look over these?

== Change summary for alert #7830 (as of July 11 2017 12:59 UTC) ==

Regressions:

  1%  compiler warnings summary linux32 pgo      1,408.33 -> 1,423.33
  1%  compiler warnings summary linux64 pgo      1,449.17 -> 1,464.00

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=7830
Flags: needinfo?(mak77)
(In reply to Ionuț Goldan [:igoldan], Performance Sheriffing from comment #17)
>   1%  compiler warnings summary linux32 pgo      1,408.33 -> 1,423.33
>   1%  compiler warnings summary linux64 pgo      1,449.17 -> 1,464.00

Interesting, do we have a diff showing the new warnings? It may be handy for these cases.
Otherwise I'll just look at a log for the pgo build.
Unfortunately, we don't have these diffs yet. Bug 1367579 has been openned just for that, but it's WIP. If this is hard to figure out or would take longer to fix, you can ignore comment 17.
Flags: needinfo?(mak77)
Depends on: 1380302
I filed bug 1380302 to handle the new warnings.
I saw you resolved the compiler warnings issue. Thank you!

== Change summary for alert #7901 (as of July 13 2017 06:46 UTC) ==

Improvements:

  1%  compiler warnings summary linux32 pgo      1,423.42 -> 1,407.25
  1%  compiler warnings summary linux64 pgo      1,464.08 -> 1,448.00

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=7901
Blocks: 1361091
You need to log in before you can comment on or make changes to this bug.