Closed
Bug 724878
Opened 12 years ago
Closed 12 years ago
Make nsPermissionManager input asynchronous
Categories
(Core :: Networking: Cookies, defect)
Core
Networking: Cookies
Tracking
()
RESOLVED
FIXED
mozilla18
People
(Reporter: Yoric, Assigned: mounir)
References
(Blocks 1 open bug)
Details
(Keywords: main-thread-io, perf, Whiteboard: [Snappy:P2])
Attachments
(2 files)
4.13 KB,
text/plain
|
Details | |
7.05 KB,
patch
|
justin.lebar+bug
:
review+
|
Details | Diff | Splinter Review |
Followup bug to bug 702344.
Reporter | ||
Comment 1•12 years ago
|
||
The objective of this bug is to make nsPermissionManager input asynchronous. I hope that we can simply make _initialization_ asynchronous and further calls synchronous.
Comment 2•12 years ago
|
||
(In reply to David Rajchenbach Teller [:Yoric] from comment #1) > The objective of this bug is to make nsPermissionManager input asynchronous. > I hope that we can simply make _initialization_ asynchronous and further > calls synchronous. what are the implications for this for snappy?
Reporter | ||
Comment 3•12 years ago
|
||
From what I understand of my conversations with mak, nsPermissionManager IO takes place in permanence when we request a page. I understand that making it fully asynchronous should make page loading more responsive. I have not attempted to investigate this part further yet, so I may be wrong.
Updated•12 years ago
|
Whiteboard: [Snappy] → [Snappy:P2]
Comment 4•12 years ago
|
||
I instrumented a build and tested on a macbook. Init takes 3 ms, all other operations takes less than a ms, generally a lot less. UpdateDB and AddInternal sometimes get close to 1 ms, but that's about it. So thankfully this does not look so bad at first glance (I recall it was pretty bad on e10s mobile last year, due to lots of cross-process traffic, but that is no longer an issue there anyhow). With an old profile and an old permissions database file, things might be different though.
Comment 5•12 years ago
|
||
We have slow sql telemetry data. Any IO operation is likely to take a few seconds at some point.
Comment 6•12 years ago
|
||
Doing engineering to make operations asynchronous and so forth can definitely help with the bad cases that can happen. We could also attack this from another direction, though: We could make sure that the datafiles we have are within sane limits, like make sure the files are within reasonable size, prune old data, etc. My suspicion is that if the permissions database is "reasonable", then we probably don't need to rewrite the manager to be asynchronous (data in comment 4 seems to support that). And if it is "unreasonable" then making it asynchronous would help but not completely fix the problem either.
Comment 7•12 years ago
|
||
(In reply to Alon Zakai (:azakai) from comment #6) > Doing engineering to make operations asynchronous and so forth can > definitely help with the bad cases that can happen. We could also attack > this from another direction, though: We could make sure that the datafiles > we have are within sane limits, like make sure the files are within > reasonable size, prune old data, etc. > > My suspicion is that if the permissions database is "reasonable", then we > probably don't need to rewrite the manager to be asynchronous (data in > comment 4 seems to support that). And if it is "unreasonable" then making it > asynchronous would help but not completely fix the problem either. I used to think this way too. However various telemetry has shown that IO has nasty perf outliers and as long as we have main thread IO firefox UI will jank horribly during large downloads, background IO activity from other apps, etc. To achieve smooth UX, we need to get rid of all main thread IO.
I guess this is the right bug for this? This happened while I was repo sync'ing a b2g checkout. Firefox went totally unresponsive for about 30s and then I attached with gdb. I see this fairly often when building gecko or sync'ing repos, was able to catch in gdb this time. As Taras notes, there's no such thing as "sync IO is OK enough ...", because you're dealing with a physical device with basically unbounded wait times for operations (unlike, say, executing a CPU instruction or reading RAM). Citing measurements taken from idle desktop-class machines is totally bogus.
Assignee | ||
Comment 9•12 years ago
|
||
This is making insertions, deletions and removals from the PermissionManager asyncs. The only sync calls to the DB happens during initialization and re-initialization. Having this async is less obviously important because we will have to block anyway as soon as we will have to read the DB and the amount of work to make that async isn't low. I would recommend making that part a follow-up.
Assignee | ||
Comment 10•12 years ago
|
||
Comment on attachment 657517 [details] [diff] [review] Patch Ideally, I would have Yoric to review this but he might be offline very soon for an extended period of time. Taras, Yoric is offline, could you review this? :)
Attachment #657517 -
Flags: review?(taras.mozilla)
Attachment #657517 -
Flags: review?(dteller)
Attachment #657517 -
Flags: review?
Assignee | ||
Comment 11•12 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=70055ed2e040
Reporter | ||
Comment 12•12 years ago
|
||
Comment on attachment 657517 [details] [diff] [review] Patch Review of attachment 657517 [details] [diff] [review]: ----------------------------------------------------------------- Canceling review for the moment, as I will not be in position to review this before a few weeks. Mounir, if you still need that review when I return, please feel free to r? me again.
Attachment #657517 -
Flags: review?(dteller)
Comment 13•12 years ago
|
||
Comment on attachment 657517 [details] [diff] [review] Patch Sorry to bounce you again. I don't own this code, my review would not help in getting this landed it.
Attachment #657517 -
Flags: review?(taras.mozilla) → review?(josh)
Comment 14•12 years ago
|
||
Comment on attachment 657517 [details] [diff] [review] Patch I am certainly not the owner of this code.
Attachment #657517 -
Flags: review?(josh) → review?(mak77)
Assignee | ||
Comment 15•12 years ago
|
||
Comment on attachment 657517 [details] [diff] [review] Patch Review of attachment 657517 [details] [diff] [review]: ----------------------------------------------------------------- I doubt anyone really owns this code. Jonas already reviewed a bunch of PermissionManager patches. Maybe he can review that one.
Attachment #657517 -
Flags: review?(jonas)
Assignee | ||
Comment 16•12 years ago
|
||
Comment on attachment 657517 [details] [diff] [review] Patch Review of attachment 657517 [details] [diff] [review]: ----------------------------------------------------------------- Or Justin?
Attachment #657517 -
Flags: review?(justin.lebar+bug)
Comment 17•12 years ago
|
||
Comment on attachment 657517 [details] [diff] [review] Patch I've got this.
Attachment #657517 -
Flags: review?(mak77)
Attachment #657517 -
Flags: review?(jonas)
Comment 18•12 years ago
|
||
Comment on attachment 657517 [details] [diff] [review] Patch > void > nsPermissionManager::UpdateDB(OperationType aOp, >- mozIStorageStatement* aStmt, >+ mozIStorageAsyncStatement* aStmt, > int64_t aID, > const nsACString &aHost, > const nsACString &aType, > uint32_t aPermission, > uint32_t aExpireType, > int64_t aExpireTime, > uint32_t aAppId, > bool aIsInBrowserElement) I'm tempted to say you should re-indent this, but whatever, I think this kind if indentation is annoying. :) > nsresult RemoveAllInternal(bool aNotifyObservers); > nsresult RemoveAllFromMemory(); > nsresult NormalizeToACE(nsCString &aHost); > static void UpdateDB(OperationType aOp, >- mozIStorageStatement* aStmt, >+ mozIStorageAsyncStatement* aStmt, > int64_t aID, > const nsACString &aHost, > const nsACString &aType, > uint32_t aPermission, > uint32_t aExpireType, > int64_t aExpireTime, > uint32_t aAppId, > bool aIsInBrowserElement); Similarly here if you want to be persnickety.
Attachment #657517 -
Flags: review?(justin.lebar+bug) → review+
Assignee | ||
Comment 19•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/f50654b6b00a This is making the permission manager doing only async DB access except for the initialization parts. Given that you want to have the memory reflecting the DB before doing any action with the permission manager, making the permission manager initialization async would be quite more work and might have a smaller impact. I'm going to open a follow-up for that part.
Flags: in-testsuite-
Target Milestone: --- → mozilla18
Version: unspecified → Trunk
Comment 20•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/f50654b6b00a
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Comment 21•12 years ago
|
||
Comment on attachment 657517 [details] [diff] [review] Patch Review of attachment 657517 [details] [diff] [review]: ----------------------------------------------------------------- ::: extensions/cookie/nsPermissionManager.cpp @@ +1512,4 @@ > } > > + nsCOMPtr<mozIStoragePendingStatement> pending; > + rv = aStmt->ExecuteAsync(nullptr, getter_AddRefs(pending)); Making these asynchronous without changing the permission manager API seems destined to create race conditions. Before, a postcondition of setting a permission was that immediately getting that same permission would give you the same value you set. Now, that is not the case. IMO it would have been better to create new *Async versions of the public methods and then deprecate (or immediate remove) the original synchronous versions.
Assignee | ||
Comment 22•12 years ago
|
||
(In reply to Brian Smith (:bsmith) from comment #21) > Comment on attachment 657517 [details] [diff] [review] > Patch > > Review of attachment 657517 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: extensions/cookie/nsPermissionManager.cpp > @@ +1512,4 @@ > > } > > > > + nsCOMPtr<mozIStoragePendingStatement> pending; > > + rv = aStmt->ExecuteAsync(nullptr, getter_AddRefs(pending)); > > Making these asynchronous without changing the permission manager API seems > destined to create race conditions. Before, a postcondition of setting a > permission was that immediately getting that same permission would give you > the same value you set. Now, that is not the case. This is not exactly true. All permission operations are still synchronous. That means, if you add a permission and check if the permission is there, you will get the expected result. The reason is because nsPermissionManager has a memory representation of the permissions and that part is updated synchronously every time there is an operation and this is used to tests. The database is only used to initialise the permission manager. We don't read anything from it except to initialise the memory representation of the permissions. The other side effect I can see from that change is that if you crash after modifying a permission, you might not get that change at reboot (because the db write wasn't finished). I wouldn't say this is a real issue. By the way, this is exactly why the Init() part of the permission manager is still synchronous: the only way to make it asynchronous without breaking our current code is to have all other operations blocking if the init isn't done, which, might be equivalent of making the init synchronous because I think the initialisation is done lazily when we get the permission service and most of the time, we get it to make an operation. What we could do though is to no longer make the initialisation lazy and init at startup eagerly so we have high chances to get the init done when we actually use the permission manager. That might not work, that might slow startup speed so this clearly need investigation.
You need to log in
before you can comment on or make changes to this bug.
Description
•