1465986 - Permission Manager initialized the DB on profile startup

Status: RESOLVED DUPLICATE of bug 1363541

(Core :: Permission Manager, enhancement, P2)

Description

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

I noticed on arewesmoothyet.com a hang caused by the fact permission manager calls InitDB(...) on profile-do-change, it also calls it in Init()... why doesn't it open the db connection on the first actual read/write API request?
Is the first request likely to arrive immediately on profile startup?

(there's still the fact this is synchronous, but that's bug 975996)

Comment 1

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

note this was in the Sever Hangs (>2s) list

Comment 2

[Alex Thayer [:alexical] (she/her)]

Taking a look into this.

Comment 3

[Alex Thayer [:alexical] (she/her)]

Am I misunderstanding this or is this effectively a dupe of Bug 1363541?

Comment 4

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

yes, it looks very related, though I find surprising that such bug doesn't tell that the permission manager initialized the db connection in profile-do-change, rather than doing that lazily when necessary.

Basically this is a subset of Bug 1363541, that bug is looking at delaying the init of the service, while here I was just pointing out the fact we could delay the init of the database. Whether this is effective or not is TBD, if the first READ/WRITE happens immediately we may not have a gain, but if the things initialing the service don't actually use it immediately, it could move the first I/O a bit later.

Comment 5

[Alex Thayer [:alexical] (she/her)]

From my understanding the best we could do without significant work is delay the init of the db if the database file doesn't exist. The first read happens immediately, inside the InitDB method, in order to get the permissions which we pass to the content process on creation (if we were to find a good way to delay that read, we'd still do a read immediately from NotificationTelemetryService::RecordPermissions()). If the database file doesn't exist, and the hostperm.1 file doesn't exist, then we don't need to do any IO other than those exists checks in order to give the content process what it needs.

In my testing the first write doesn't happen until the user is actually prompted to grant a permission for something, so it may be that a new user could run Firefox quite a few times without having to ever create the DB, but after they granted permissions for one thing, startup would be slower for them.

This would also all come at the cost an extra file exists() call, which is not nothing, so I'm inclined to say that this should just be wrapped under Bug 1363541. Thoughts?

Comment 6

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

If it's used that early, I agree to just close this and keep tracking Bug 1363541.

What I found strange here was finding a stack where the db was inited on profile-do-change (unfortunately I can't find that anymore on arewesmoothyet, maybe someone removed a really early init of the service in recent startup perf work). There's likely no good reason to do that, when one could just have an "ensureConnection" method invoked to build the connection lazily when necessary, it looked like initializing the connection on profile change just to have it readily available later.