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

Status: RESOLVED FIXED

(Toolkit :: Places, defect, P1)

Description

[Marco Bonardo [:mak]]

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

Comment 1

[Marco Bonardo [:mak]]

Attachment: Bug 1371677 - Delay the database connection in the history service as far as possible.

Review commit: https://reviewboard.mozilla.org/r/147652/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/147652/

Comment 2

[Marco Bonardo [:mak]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/147652/diff/1-2/

Comment 3

[Florian Quèze [:florian]]

Attachment: Test

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

Comment 4

[Marco Bonardo [:mak]]

(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.

Comment 5

[Marco Bonardo [:mak]]

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.

Comment 6

[Florian Quèze [:florian]]

(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.

Comment 7

[Marco Bonardo [:mak]]

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

Comment 8

[Marco Bonardo [:mak]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/147652/diff/2-3/

Comment 9

[Marco Bonardo [:mak]]

Comment on attachment 8876863 [details] [diff] [review]
Test

The startup test changes landed already, so this is no more needed.

Comment 10

[Marco Bonardo [:mak]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/147652/diff/3-4/

Comment 11

[Marco Bonardo [:mak]]

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

Comment 12

[Drew Willcoxon :adw]

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

Comment 13

[Drew Willcoxon :adw]

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?

Comment 14

[Marco Bonardo [:mak]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/147652/diff/4-5/

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/43bfc32f2623
Delay the database connection in the history service as far as possible. r=adw

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/43bfc32f2623

Comment 17

[Ionuț Goldan [:igoldan]]

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

Comment 18

[Marco Bonardo [:mak]]

(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.

Comment 19

[Ionuț Goldan [:igoldan]]

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.

Comment 20

[Marco Bonardo [:mak]]

I filed bug 1380302 to handle the new warnings.

Comment 21

[Ionuț Goldan [:igoldan]]

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