Closed Bug 1371677 Opened 3 years ago Closed 3 years ago

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

Categories

(Toolkit :: Places, defect, P1)

55 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: mak, Assigned: mak)

References

(Blocks 1 open bug)

Details

(Keywords: perf, Whiteboard: [fxsearch])

Attachments

(1 file, 1 obsolete file)

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).
Whiteboard: [fxsearch]
Attached patch Test (obsolete) — Splinter Review
Attaching here the test changes I had initially made for bug 1371710 in case it is helpful.
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).
No longer depends on: 1371945
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
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
Keywords: perf
Blocks: 1377600
Blocks: 1377599
Blocks: 1377598
Sorry for not reviewing this yet, Marco.  I'll try to finish it ASAP.
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+
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
https://hg.mozilla.org/mozilla-central/rev/43bfc32f2623
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
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
Blocks: 1436966
You need to log in before you can comment on or make changes to this bug.