Closed
Bug 870460
Opened 12 years ago
Closed 7 years ago
Lazy load of cookie service blocks main thread while cookie database loads
Categories
(Core :: Networking: Cookies, defect, P2)
Core
Networking: Cookies
Tracking
()
Tracking | Status | |
---|---|---|
firefox58 | --- | fixed |
People
(Reporter: jduell.mcbugs, Assigned: CuveeHsu)
References
(Depends on 1 open bug, Blocks 1 open bug)
Details
(Keywords: perf, Whiteboard: [necko-quantum][necko-triaged])
Attachments
(4 files, 12 obsolete files)
(deleted),
patch
|
CuveeHsu
:
review+
francois
:
feedback+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
CuveeHsu
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
jmaher
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
CuveeHsu
:
review+
|
Details | Diff | Splinter Review |
Desktop currently lazily loads the cookie database (i.e. we wait until the first use of the DB to initialize the cookie service). So the first HTTP channel that does an AsyncOpen will call GetCookieStringFromHttp (a synchronous call), which winds up waiting for the cookie service to open the database and fetch the desired cookie synchronously off disk. Even if we make all the I/O in the cookie DB off-main-thread and/or async, we'd still have to have the main thread sit and wait for it since the GetCookie API is sync. I suspect the best fix is to init the cookie service earlier in startup, loading it async (or sync on a non-main thread) and in most cases it will hopefully already be loaded by the time it's used. We'd have to measure if this is actually a win, though. Measurement here is a little tricky. We could instrument the code so we get telemetry on the 1st cookie load, and compare that time. If I'm right it will be faster (hopefully instant) when we load the cookie service earlier. But this won't measure the impact that loading the cookie service earlier has on startup time overall (there will be some possibility that moving the cookie I/O earlier will cause I/O contention and slow startup). Not sure if there's a good way to measure that. Taras, does this sound like a plan that's worth trying? It's a simple enough patch to write (as long as someone can point me at a good place during startup to init the cookie service). Note that in b2g we already do the early init (bug 810209) to avoid delete races.
Comment 2•12 years ago
|
||
Can we make the network channel wake up the cookie db and have the main thread wait for the channel as it usually waits for data to load?
Flags: needinfo?(taras.mozilla)
Reporter | ||
Comment 3•12 years ago
|
||
If by "wait" you mean "wait asynchronously", we can do that, but only by changing the nsICookieService API to add an async version of GetCookie. Doable, and we would presumably not have to change all call sites, just nsHttpChannel::AsyncOpen. It's doable. Of course the first pageload will still be delayed by however long it takes to load the SQL.
Updated•9 years ago
|
Whiteboard: [necko-backlog]
Updated•7 years ago
|
Reporter | ||
Comment 6•7 years ago
|
||
So in bug 1360202 we're seeing startup jank of 90ms, which would be good to fix (I'm not sure I'd put it as a p1 myself, but I'm hoping the fix is easy.) Per comment 0--we need to find a suitable, early-as-possible place to initialize the cookie service, instead of our current dumb lazy-load strategy. If we do that, with any luck we'll be done with the async query that loads the whole SQLite cookie database into memory. And if not, at least the amount of time we'll need to wait for it should be less. Benjamin, where's a good place to put this during init? AFAIK the cookie service shouldn't need a whole lot of other stuff running already (SQLite being the big one). Nick, if you're busy you could possibly give this to Junior. But it looks relatively easy and self-contained.
Assignee: nobody → hurley
Flags: needinfo?(benjamin)
Whiteboard: [necko-backlog] → [qf:p1][necko-quantum] [necko-active]
Comment 8•7 years ago
|
||
(In reply to Jason Duell [:jduell] (needinfo me) from comment #6) > So in bug 1360202 we're seeing startup jank of 90ms, which would be good to > fix (I'm not sure I'd put it as a p1 myself, but I'm hoping the fix is easy.) > > Per comment 0--we need to find a suitable, early-as-possible place to > initialize the cookie service, instead of our current dumb lazy-load > strategy. If we do that, with any luck we'll be done with the async query > that loads the whole SQLite cookie database into memory. And if not, at > least the amount of time we'll need to wait for it should be less. > > Benjamin, where's a good place to put this during init? AFAIK the cookie > service shouldn't need a whole lot of other stuff running already (SQLite > being the big one). Note if you want to use sqlite as early as possible, there is one potential blocker bug 730495 that we don't have a well defined "sqlite is ready" point where we can safely call sqlite API.
Comment 9•7 years ago
|
||
(In reply to Jason Duell [:jduell] (needinfo me) from comment #6) > So in bug 1360202 we're seeing startup jank of 90ms, which would be good to > fix (I'm not sure I'd put it as a p1 myself, but I'm hoping the fix is easy.) For the 90ms jank, I'd like to share more info for reference: I just tried to profile my old mac mini(2010 Mid) with the latest m-c and it spent 130ms for nsCookieService::Init() 110ms for nsCookieService::InitDBStates() the profile is here: https://perfht.ml/2qq6blw Bug 1356605 comment 10 also shows that a very different result for the profiling in InitDBStates() (9ms vs 90ms) I think it might be mostly about the SSD vs HDD.
Reporter | ||
Comment 10•7 years ago
|
||
kanru: so I get from bug 730495 that there's no *programmatic* way to guarantee that sqlite is ready. But we obviously must have some places in our startup at this point where we know that it effectively must be ready (i.e. where we can start using places, NSS, etc). Do you know of the earliest point when that is (or at least some early point)?
Flags: needinfo?(kchen)
Comment 11•7 years ago
|
||
(In reply to Jason Duell [:jduell] (needinfo me) from comment #10) > kanru: so I get from bug 730495 that there's no *programmatic* way to > guarantee that sqlite is ready. But we obviously must have some places in > our startup at this point where we know that it effectively must be ready > (i.e. where we can start using places, NSS, etc). Do you know of the > earliest point when that is (or at least some early point)? It was working because coincidence. I think it was https://dxr.mozilla.org/mozilla-central/rev/1ec3a3ff68f2d1a54e6ed33e926c28fee286bdf1/toolkit/mozapps/extensions/internal/XPIProvider.jsm#1250-1278 initializing sqlite but the line was removed by bug 1277295. I'm not sure where sqlite is initialized now without using a debugger. Really, I think we should fix bug 730495 properly...
Flags: needinfo?(kchen)
Reporter | ||
Comment 12•7 years ago
|
||
OK, I'll ask in bug 730495 if it's likely to happen in the 57 time frame. I'm guessing not, so perhaps we'll just need to fire up a debugger for now and find a place to insert a cookieservice init.
Reporter | ||
Comment 13•7 years ago
|
||
Marco: So currently our cookieService runs the following SQLite logic during startup. Besides our plan to simply start this logic earlier, I'm wondering what else we can do here: ------ InitDBStates(): ReadAheadFile(mDefaultDBState->cookieFile); mStorageService->OpenUnsharedDatabase(mDefaultDBState->cookieFile, insert a bunch of DB Listeners TableExists() check GetSchemaVersion() 3 CreateAsyncStatement()s Call Read() 2 more CreateAsyncStatement()s // Start a new connection for sync reads, to reduce contention with the // background thread [JD: not sure what that means?]. We need to do this // before we kick off write statements, since they can lock the database // and prevent connections from being opened. [JD: still true?] OpenUnsharedDatabase() ExecuteAsync(SELECT * from the cookie table) ExecuteAsync(possibly no longer needed deletion of any records with NULL origin) --- Which of these things block? I'm assuming the major delay is calling OpenUnsharedDatabase. It doesn't look like we can avoid that call (i.e switch to OpenAsyncDatabase() instead), since we need to check the schemaVersion of the database at startup, and that operation is only supported for synchronous connections (could that easily be changed? Could we have an AsyncGetSchemaVersion (with a callback) for async connections? Also, if you have any thoughts on a good, early place to init the cookieSvc during startup (see comment 6) that would be great too.
Flags: needinfo?(mak77)
Comment 14•7 years ago
|
||
(In reply to Jason Duell [:jduell] (needinfo me) from comment #13) > Which of these things block? Everything but ExecuteAsync. > mStorageService->OpenUnsharedDatabase(mDefaultDBState->cookieFile, For sure this one is the biggest offender. > TableExists() check > GetSchemaVersion() These are basically just tiny wrappers around queries, they are unlikely to do IO since this info is probably in memory, though they are on main-thread and thus they contend the database resource with the async thread (if the async thread is busy doing anything, the main-thread blocks). Should not be a problem in your current setup, since everything before them is synchronous... > I'm assuming the major delay is calling > OpenUnsharedDatabase. It doesn't look like we can avoid that call (i.e > switch to OpenAsyncDatabase() instead), since we need to check the > schemaVersion of the database at startup, and that operation is only > supported for synchronous connections Actually GetSchemaVersion() is just a wrapper that executes a "PRAGMA user_version" statement and fetches the result... You can do the same with ExecuteAsync. We didn't add an API because through the current API you can do that already. We could also gain an helper if that helps, but likely you'll have to do that yourself, there's no Storage dedicated resources, just an owner and some peers taking care of needinfo, updates and emergencies. Also TableExists() is a simple SELECT against sqlite_master. Btw, it was my assumption that the cookies manager is synchronous, so you'll still have to block the main-thread until the db is ready? Surely by opening early you could delay that, but it will then regress startup. Imho the cleaner solution would be to expose 2 APIs, one asynchronous in the chrome process, the other synchronous in the content process. Content can use a ScriptBlocker. That would allow to keep the UI responsive and then it wouldn't matter much when you init cookies. > Also, if you have any thoughts on a good, early place to init the cookieSvc > during startup (see comment 6) that would be great too. IIRC (it passed some time so I may have rusty memories) bug 730495 should not be a problem for you. That bug is about NSS initializing the sqlite library before mozStorage. Your code always goes through mozStorage, so it should not be affected afaict, unless you also initialize NSS before the database in your code. By making your code running earlier off-hand looks like you will reduce the likely for bug 730495 to happen, rather than causing it.
Flags: needinfo?(mak77)
Comment 15•7 years ago
|
||
So I've run this under a debugger, and everything said in comment 14 jives with what I saw - initializing the cookie service causes mozstorage to be initialized, which calls sqlite3_config. So we're all good on the race, we'll never hit it no matter when we init the cookie service. The question at this point, Jason, comes down to would we rather (1) slow down startup (by initing the cookie service, say, when we start up the socket thread), or (2) add a new async cookie api to always be used internally by gecko, and make the sync api use the scriptblocker thing. The latter is definitely way more work (I haven't looked into what would be involved yet), and I have to imagine it's been discussed at some point in the past (see also how we had both sync and async cache APIs) and decided against, but this may change the calculus. In the long run, though, the async api option could quite possibly be The Right Thing.
Flags: needinfo?(jduell.mcbugs)
Reporter | ||
Comment 16•7 years ago
|
||
The silly thing about making the cookie API async is that it doesn't provide any benefit except during startup. And I suspect it wouldn't even help startup potentially--all it takes is some page JS checking document.cookie and we'll have to fall back to a sync option. I think it might be better and easier to have the cookieService attempt to load the database with the OpenAsync() version very early on in startup: 1) OpenAsyncDatabase() 2) in callback when DB is ready, asyncExecute Table Exists check. 3) In callback, if table does exist, asyncExecute("PRAGMA user_version") 4) In that callback, check if the schema is the right version. 5) If either the table was missing, or scheme version is off, do fall back logic--open a sync DB connection and do all the grunt work to re-create the tables (sucks to do it synchronously, but it's a rare failure pathway.) 4) If the schema version was OK, asyncExecute the big select * that loads all the cookies into memory. 5) When we actually get our first GetCookie() etc call on the cookieService, hopefully we'll have all the data already (telemetry!). If we don't, open a sync database connection and ask for just the one record in question (i.e. the logic that we currently have for GetCookie(), except we'd only open the sync connection if and when it's needed). I.e. I think if we get lucky we can use only async APIs for the initial database read, which will get rid of the startup jank, and only do sync database calls when we hit a bad or out of date database. This is a fairly simple plan, but there's probably some complexity/risk, and it might not hit 57 (or might take away dev hours we could more profitably use elsewhere). Hard to say exactly at this point. Maybe give it a stab for a day or two and if it winds up being harder than we think, simply moving the DB init earlier in startup may be all we can do for 57.
Flags: needinfo?(jduell.mcbugs)
Reporter | ||
Comment 17•7 years ago
|
||
Another option (which I just thought of, so maybe it's half-baked): move all cookie DB access to a new thread (or maybe just use one from our thread pool for blocking disk I/O). It can use sync APIs for everything (including the current async "select *"). Fire off init early in startup. When GetCookie gets called, if we haven't finished loading the cookies into the big hashtable, block the main thread on a mutex until we're done. Present telemetry to show that the blocking hardly ever happens and point out that even if it does, the alternative is to block on I/O on the main thread. This might be cleaner and easier to implement (and moves all the I/O off the main thread, except for the sometimes hitting a mutex issue).
Comment 18•7 years ago
|
||
(In reply to Jason Duell [:jduell] (needinfo me) from comment #16) > And I suspect it wouldn't even help > startup potentially--all it takes is some page JS checking document.cookie > and we'll have to fall back to a sync option. The idea would be that at point you don't block the main-thread waiting for init, you block the page with a scriptBlocker instead.
![]() |
||
Comment 19•7 years ago
|
||
(In reply to Marco Bonardo [::mak] from comment #18) > (In reply to Jason Duell [:jduell] (needinfo me) from comment #16) > > And I suspect it wouldn't even help > > startup potentially--all it takes is some page JS checking document.cookie > > and we'll have to fall back to a sync option. > > The idea would be that at point you don't block the main-thread waiting for > init, you block the page with a scriptBlocker instead. Exactly. OTOH, how is the <head/><body><script>var allCookies = document.cookie;</script>.... case handled?
Comment 20•7 years ago
|
||
Looks to me as if other people answered this better than I could have. Let me know if you still have questions for me.
Flags: needinfo?(benjamin)
Reporter | ||
Comment 21•7 years ago
|
||
After talking this through with Nick, our plan is to go with comment 17: we're going to move all SQLite activity onto a new thread. - We'll put code that initializes the cookieService (i.e. just grab it to a nsCOMPtr but then don't actually do anything with it) early on somewhere during startup (Nick suggests we do it when we initialize the socket transport thread: I'd like to ask around to find out if that's the earliest spot). I'm not sure if we need to do this init on the main thread or not (the code will just launch a new thread, so maybe it doesn't matter which thread uses it--but generally the cookieService is main-thread only and I don't know if there's any XPCOM service infrastructure that requires that services are requested only on the main thread). - When the cookieService is initialized it will create a new thread that does all the current logic to open (and if needed, alter/reconstruct) the cookie database. One change: we can get rid of the current logic that does a blocking sync request for a single cookie if the large "select all cookies" async query hasn't finished. Instead we can make the "select all" a sync query (and just get rid of the single cookie request). - If something on the main thread calls into the cookieService before it is finished setting up the hashtable (i.e. the sqlite query isn't complete), we can either 1) simply block the main thread on a monitor until it's done, and gather telemetry to see how often it happens (and with how much delay). If those numbers show any significant jank then we should 2) add an async version of GetCookie, and have necko use it in AsyncOpen, so we don't block the main thread. - We will need to make sure that *all* database access happens on the new "cookie I/O thread", so it will stay alive after initialization, and any calls to SetCookie/DeleteCookie/etc will need to 1) modify the hashtable synchronously on the main thread (see comment on locking, below), and then 2) proxy an event to the cookie I/O thread to do the SQLite update. Locking: so in this plan we'll have two different threads modifying the cookie hashtable. The most obvious way to handle that would be to have them lock a mutex every time they need hashtable access. But the write pattern here is actually very simple, and we might not need to lock/unlock a mutex for every cookieService access. We basically have a two-stage access model: in stage 1 (initialization), only the cache I/O thread is allowed to write (or read) the hashtable, and if the main thread tries to, it must be delayed (via monitor.wait) until init is complete. In stage 2 (post-init), the only thread that will read or write (AFAICT) the hashtable will be the main thread (the cache I/O thread will do writes to the database, but shouldn't need to touch the hashtable after init AFAICT). So we might be able to get away with keeping some "mInitialized" boolean that starts out at 'false' and only gets set to 'true' after the hashtable has been written AND we've guaranteed that we've flushed the CPU operations to memory (Any mutex/monitor lock or unlock will flush memory). So the I/O thread would do something like // It's OK that we hold this lock while we do I/O on our non-main I/O thread MonitorAutoLock lock(mMonitor); ... Do synchronous "Select all" on database and populate the cookie hashtable mInitialized = true; mMonitor.Notify(); And then all synchronous CookieService functions like Get|SetCookie could then do something like if (mInitialized) { touch the hashtable, no need to lock } else { MonitorAutoLock lock(mMonitor); while (!mInitialized) { mMonitor.Wait(); } } This could be wrong, and/or overkill (maybe grabbing a mutex every time we call into the CookieService is cheap enough). I'll ask someone who knows our thread synchronization stuff better than I do.
Reporter | ||
Comment 22•7 years ago
|
||
Nathan: can you look at comment 21 and tell me 1) If you know of a good, early place during startup to initialize the cookieService (requires SQLite to be working/available by then). 2) Does service instantiation need to happen on the main thread? 3) Does my plan for lockless access to the cookie hashtable seem sane?
Flags: needinfo?(nfroyd)
Reporter | ||
Comment 23•7 years ago
|
||
Note: in case I didn't make it clear, this plan would mean that *all* of our cookie SQLite would be done with a sync db connection on the I/O thread. AFAICT we would no need to do anything async, and would no longer need to open a 2nd, async db connection.
Comment 24•7 years ago
|
||
Junior - Jason says you're free to take this on. I'll help mentor you through it and do at least the first round or two of reviews. Sound good?
Assignee: hurley → juhsu
Assignee | ||
Comment 25•7 years ago
|
||
(In reply to Nicholas Hurley [:nwgh][:hurley] (also hurley@todesschaf.org) from comment #24) > Junior - Jason says you're free to take this on. I'll help mentor you > through it and do at least the first round or two of reviews. Sound good? Yes, I'm willing to take this. It's great to have your help with the bug.
![]() |
||
Comment 26•7 years ago
|
||
Sorry for the delay here... (In reply to Jason Duell [:jduell] (needinfo me) from comment #22) > 1) If you know of a good, early place during startup to initialize the > cookieService (requires SQLite to be working/available by then). Do we have a single place that initializes SQLite? I guess this would be the storage service, which needs to be instantiated on the main thread. And the cookie service uses that. So we'd need to find a good place to (transitively) start up the storage service; I guess NS_InitXPCOM2 or nsLayoutStatics::Initialize (called from NS_InitXPCOM2) would be early enough. > 2) Does service instantiation need to happen on the main thread? Service instantiation does not need to happen on the main thread. That being said, a lot of our services do assert that they are created on the main thread, have single-threaded refcounting, and/or are used only on the main thread. > 3) Does my plan for lockless access to the cookie hashtable seem sane? I think so; all modifications to the cookie service hashtable will happen from a single thread after the initial load? TSan will probably complain about the pattern, so that's a clear minus, but there are ways around that.
Flags: needinfo?(nfroyd)
Reporter | ||
Comment 27•7 years ago
|
||
> TSan will probably complain about the pattern
We could also probably just use a lock every time we enter the cookie service. IIRC it's pretty cheap to grab an uncontested lock--an atomic operation and/or a memory flush is unlikely to cost us much given how infrequently we'll be doing this
Comment 28•7 years ago
|
||
Just to remind everybody that we care a lot about this getting fixed - here is a first-startup (ie. creating the profile) profile where nsCookieService::Init() blocks the main thread for 334ms when doing the first async XHR: https://perfht.ml/2sfMzSK
Assignee | ||
Comment 29•7 years ago
|
||
(In reply to Florian Quèze [:florian] [:flo] from comment #28) > Just to remind everybody that we care a lot about this getting fixed - here > is a first-startup (ie. creating the profile) profile where > nsCookieService::Init() blocks the main thread for 334ms when doing the > first async XHR: https://perfht.ml/2sfMzSK Thanks for your reminder. Nick and I had a long discussion about this several hours ago. It could be a monster change, but it's ongoing. We had a plan and will have an experiment result in one or two weeks. Stay tuned.
Reporter | ||
Comment 30•7 years ago
|
||
After talking to Nfroyd and ehsan, it looks like the first place we should try to init the cookie DB is during the socket transport service init: -- I guess we'll kick the init off when the socket transport starts up (seems fairly early), and see how it goes. We'll be measuring how long the 1st HTTP request takes to get its cookies (ideally we want to make sure we init early enough that the answer will be 0). Hopefully if we slow down other important bits of startup that'll get captured by some existing metric and people will come hassle us (and we can possibly move the cookie init later--we can certainly experiment with how late we can fire off init while still keeping 1st-cookie-wait to 0. We could also possibly switch to an async GetCookie API, which would still cause channels to be delayed till the cookies are loaded, but at least would stop janking the main thread when it that happens).
Reporter | ||
Comment 31•7 years ago
|
||
Florian: re: comment 28. I looked at the gecko profiler report you posted--it turns out that this happens only when we need to create the tables for the database (i.e either 1st time profile, or rebuild of database). Those are rare, so that's good. I'd be surprised if Init for a regular, already created database would take that long. OTOH the next part of the cookie logic may be expensive: after HttpBaseChannel::AddCookiesToRequest() grabs the cookie services, which is the :Init() timing you posted, it then calls GetCookieStringFromHttp(), which should also cause a fair amount of jank (it needs to block on disk I/O while we load the cookies from disk). In general I'd expect the loading cookies from disk part of a profile to actually take longer than the database init part of it. (Not in this comment 28 case--with a new profile, all the work is in creating the SQlite tables, and there's no cookies in it to load).
Assignee | ||
Comment 32•7 years ago
|
||
I'm thinking if it's necessary to move _all_ the sqlite operation off-main-thread. The jank is in start-up, especially with a new profile. I'd rather than OMT the TryInitDB first. What do you think, Nick?
Flags: needinfo?(hurley)
Comment 33•7 years ago
|
||
Yes, all sqlite ops should happen off main thread. However, we still need to move cookie service init earlier, and make sure we block any cookie requests until the db is initialized (moving init earlier will help ensure that is less likely to happen, and we are therefore less likely to jank the main thread). I was under the impression that this was included in what we had agreed on earlier, but perhaps I was mistaken or misremembering.
Flags: needinfo?(hurley)
Assignee | ||
Comment 34•7 years ago
|
||
Yes, we agreed that we should move all sqlite ops OMT. The idea came from Comment 21 and Comment 23. The thing is ops other than start/close don't jank much. We could open two db connections, like current implementation. Therefore, we can split to two milestones. IMO it's a possible solution to solve the start-up jank in 57. M1: 1. Move init as early as nsIOService::InitializeSocketTransportService 2. OMT the InitDBState to socket thread 3. Block main thread if we need something in hashtable and InitDBState has not done M2: 1. Make all the sqlite ops use the same connection in |InitDBState|, so all ops are sync 2. OMT them I'd like to do M1 first. If the split does not make sense, lets land all the patches in one shot.
Flags: needinfo?(hurley)
Assignee | ||
Comment 35•7 years ago
|
||
FWIW, nsIOService::Init is not the place to init cookie service. It will crash in http://searchfox.org/mozilla-central/rev/bbc1c59e460a27b20929b56489e2e55438de81fa/netwerk/cookie/nsCookieService.cpp#739 No matter we're running in main thread or socket thread. 1) Main thread would hit |nsScriptSecurityManager::Init()| and go to get IOService. bang! * frame #0: 0x00000001024b6872 XUL`nsScriptSecurityManager::Init() [inlined] nsresult CallGetService<nsIIOService>(aContractID=<unavailable>, aDestination=<unavailable>) at nsServiceManagerUtils.h:89 [opt] frame #1: 0x00000001024b6872 XUL`nsScriptSecurityManager::Init(this=0x00000001006ee2e0) at nsScriptSecurityManager.cpp:1354 [opt] frame #2: 0x00000001024b6c82 XUL`nsScriptSecurityManager::InitStatics() at nsScriptSecurityManager.cpp:1428 [opt] 2) Socket thread might say we're not ready yet. http://searchfox.org/mozilla-central/rev/bbc1c59e460a27b20929b56489e2e55438de81fa/xpcom/base/nsCycleCollector.cpp#3967
Assignee | ||
Comment 36•7 years ago
|
||
I did a NS_DispatchToCurrentThread trick in IOService::Init It's better with some random crash on db read/first httpChannel::AsyncOpen2
Comment 37•7 years ago
|
||
(In reply to Junior[:junior] from comment #34) > Yes, we agreed that we should move all sqlite ops OMT. > The idea came from Comment 21 and Comment 23. > > The thing is ops other than start/close don't jank much. > We could open two db connections, like current implementation. > > Therefore, we can split to two milestones. > IMO it's a possible solution to solve the start-up jank in 57. > > M1: > 1. Move init as early as nsIOService::InitializeSocketTransportService > 2. OMT the InitDBState to socket thread > 3. Block main thread if we need something in hashtable and InitDBState has > not done > > M2: > 1. Make all the sqlite ops use the same connection in |InitDBState|, so all > ops are sync > 2. OMT them > > I'd like to do M1 first. > If the split does not make sense, lets land all the patches in one shot. My concern here is that having this bifurcated solution will take approximately the same amount of time as OMT-ing everything, so we split the work for no time savings (in terms of when it gets landed). There's also a concern about things more easily getting out of sync using multiple connections. That said, I'll trust your judgement on timing and consistency, as you're the one writing the code, and you know better than I what kind of work this is going to take. The above are just a couple things I want to make sure we're taking into account when we take the decision here.
Flags: needinfo?(hurley)
Reporter | ||
Comment 38•7 years ago
|
||
> M1: > 1. Move init as early as nsIOService::InitializeSocketTransportService > 2. OMT the InitDBState to socket thread Don't run anything on the socket thread. I think we'll want a separate thread that just does cookie I/O. That's what I was thinking anyway. It's possible we could use the file I/O thread pool instead. But I think a dedicated thread is fine and would be simplest. > 3. Block main thread if we need something in hashtable and InitDBState has not done > > M2: > 1. Make all the sqlite ops use the same connection in |InitDBState|, so all ops are sync > 2. OMT them I'm not sure I understand what's in M1 exactly. If we're blocking main thread cookie calls until the background I/O thread finishes reading from SQLite, then what SQL would still be using the main thread SQLite connection(s)?
Flags: needinfo?(juhsu)
Assignee | ||
Comment 39•7 years ago
|
||
(In reply to Jason Duell [:jduell] (needinfo me) from comment #38) > > M1: > > 1. Move init as early as nsIOService::InitializeSocketTransportService > > 2. OMT the InitDBState to socket thread > > Don't run anything on the socket thread. I think we'll want a separate > thread that just does cookie I/O. That's what I was thinking anyway. It's > possible we could use the file I/O thread pool instead. But I think a > dedicated thread is fine and would be simplest. > OK, I'm confused previously since comment 30 discussed about socket thread. Now I guess you're thinking of the Cookie I/O thread initial time. By the way, comment 35 shows it's too early to initial as we initial socket thread. InitXPCOM2 doesn't work either since the DirectoryService can not get file correctly. I'm trying to hack XRE_mainRun later. > > 3. Block main thread if we need something in hashtable and InitDBState has not done > > > > M2: > > 1. Make all the sqlite ops use the same connection in |InitDBState|, so all ops are sync > > 2. OMT them > > I'm not sure I understand what's in M1 exactly. If we're blocking main > thread cookie calls until the background I/O thread finishes reading from > SQLite, then what SQL would still be using the main thread SQLite > connection(s)? For insert/update/remove, I'd like to keep use mDBState->dbConn, which is an async connection. No more reading from sqlite is needed, so only block the main thread.
Flags: needinfo?(juhsu)
Assignee | ||
Comment 40•7 years ago
|
||
> > I'm not sure I understand what's in M1 exactly. If we're blocking main > > thread cookie calls until the background I/O thread finishes reading from > > SQLite, then what SQL would still be using the main thread SQLite > > connection(s)? > For insert/update/remove, > I'd like to keep use mDBState->dbConn, which is an async connection. > > No more reading from sqlite is needed, so only block the main thread. 1. Mm... async connection isn't cheap enough to ignore. Sometimes SetCookieStringInternal takes several ms, even larger than InitDBState. So, eventually we need to OMT all the db operation. 2. For the initialization of nsCookieService, I believe we should wait for "profile-do-change" or "profile-after-change" http://searchfox.org/mozilla-central/rev/f0e4ae5f8c40ba742214e89aba3f554da0b89a33/netwerk/cookie/nsCookieService.cpp#1945-1962 https://developer.mozilla.org/en-US/docs/Observer_Notifications
Comment 41•7 years ago
|
||
One thing to be aware of, since the code is currently using async connections and async execution ("ExecuteAsync" calls)... minor enhancements to mozStorage are needed if you want to use ExecuteAsync and friends off the main thread. There's at least a few places in async code where we may assume the issuing thread is the main thread. The main take-aways are: - If you move code invoking ExecuteAsync() from the main thread to another thread, things may break. - It won't be very hard to fix them, but you will have to fix them.
Assignee | ||
Comment 42•7 years ago
|
||
(In reply to Andrew Sutherland [:asuth] from comment #41) > One thing to be aware of, since the code is currently using async > connections and async execution ("ExecuteAsync" calls)... minor enhancements > to mozStorage are needed if you want to use ExecuteAsync and friends off the > main thread. There's at least a few places in async code where we may > assume the issuing thread is the main thread. > > The main take-aways are: > - If you move code invoking ExecuteAsync() from the main thread to another > thread, things may break. > - It won't be very hard to fix them, but you will have to fix them. Thanks for this notice. If possible, we might merge all the connections into one sync connection.
Assignee | ||
Comment 43•7 years ago
|
||
This is a WIP patch in a very early stage, showing the idea about M1 Before I make the DB read sync, my profiler shows 1 sec between profile-do-change and first GetCookieString (except some handling in AsyncReadComplete [1]) Reading DB is fast in my environment - less than 5 ms. What do you think about the setup, Nick? TODO: 0. Make InitDBState OMT, still thinking how many things we'd like to OMT Bug 1331680 Comment 244 has a plan to init a brand new Cookie thread, which can be leveraged 1. Block the main thread if db is not read complete in EnsureReadDomain/EnsureReadComplete Probe those data to see if we need to do more. 2. Move [1] to sync read 3. Handle mDefaultDBState->readSet carefully 4. Handle DB corruption [1] http://searchfox.org/mozilla-central/rev/30a47c4339bd397b937abdb2305f99d3bb537ba6/netwerk/cookie/nsCookieService.cpp#2758-2772
Attachment #8893710 -
Flags: feedback?(hurley)
Comment 44•7 years ago
|
||
Comment on attachment 8893710 [details] [diff] [review] Part1_OMT_Init WIP1, v1 Review of attachment 8893710 [details] [diff] [review]: ----------------------------------------------------------------- First of all, sorry for the lag here. I'll do better next time (unless you happen to ask for more feedback while I'm gone the rest of this week!) Anyway, this looks like it's headed in the right direction. I do have a few concerns, though. Most of which are inline below. The big thing is that this is still pretty bare in terms of what's changed, so it's hard to get a real good sense of what the end solution looks like. I've extrapolated as best as I can for my comments. ::: netwerk/base/nsIOService.cpp @@ +1479,5 @@ > nsCOMPtr<nsIPrefBranch> prefBranch; > GetPrefBranch(getter_AddRefs(prefBranch)); > PrefsChanged(prefBranch, MANAGE_OFFLINE_STATUS_PREF); > + > + // Read cookie database This needs to explain *why* we're reading the cookie database here, preferably with a pointer to this bug for even more history. This is one of the biggest things gecko in general is missing in comments, and it's been a regular thorn in my side since I've started working here. ::: netwerk/cookie/nsCookieService.cpp @@ +1444,5 @@ > if (aRecreateDB) > return RESULT_OK; > > // check whether to import or just read in the db > + if (tableExists) { I would expect this to actually dispatch something to the IO thread. Right now, this is racy - if someone happens to call do_GetService(cookieservice) on the main thread before the new init call you added happens, then we're back in "jank the main thread" territory. @@ +2598,5 @@ > + > +void > +nsCookieService::BlockMainThreadIfUninitialized() > +{ > + //TODO It's important to know how this works before we carry on much farther :) @@ +2603,5 @@ > +} > + > +void > +nsCookieService::SyncRead() > +{ In addition to dispatching this to the IO thread (as mentioned above), this needs to assert that it's not running on the main thread as added insurance against the jank coming back accidentally in the future.
Attachment #8893710 -
Flags: feedback?(hurley)
Assignee | ||
Comment 45•7 years ago
|
||
(In reply to Nicholas Hurley [:nwgh][:hurley] (also hurley@todesschaf.org) from comment #44) > Comment on attachment 8893710 [details] [diff] [review] > Part1_OMT_Init WIP1, v1 > > Review of attachment 8893710 [details] [diff] [review]: > ----------------------------------------------------------------- > > First of all, sorry for the lag here. I'll do better next time (unless you > happen to ask for more feedback while I'm gone the rest of this week!) > > Anyway, this looks like it's headed in the right direction. I do have a few > concerns, though. Most of which are inline below. The big thing is that this > is still pretty bare in terms of what's changed, so it's hard to get a real > good sense of what the end solution looks like. I've extrapolated as best as > I can for my comments. The patch is based on Milestone 1 in Comment 34. I reuse the sync read in EnsureReadComplete, so the diff looks weird :p I'll make a better patch when you come back! > > ::: netwerk/base/nsIOService.cpp > @@ +1479,5 @@ > > nsCOMPtr<nsIPrefBranch> prefBranch; > > GetPrefBranch(getter_AddRefs(prefBranch)); > > PrefsChanged(prefBranch, MANAGE_OFFLINE_STATUS_PREF); > > + > > + // Read cookie database > > This needs to explain *why* we're reading the cookie database here, > preferably with a pointer to this bug for even more history. This is one of > the biggest things gecko in general is missing in comments, and it's been a > regular thorn in my side since I've started working here. > It's a good suggestion. Thanks! > ::: netwerk/cookie/nsCookieService.cpp > @@ +1444,5 @@ > > if (aRecreateDB) > > return RESULT_OK; > > > > // check whether to import or just read in the db > > + if (tableExists) { > > I would expect this to actually dispatch something to the IO thread. Right > now, this is racy - if someone happens to call do_GetService(cookieservice) > on the main thread before the new init call you added happens, then we're > back in "jank the main thread" territory. > Plan A is to simply block main thread and add a telemetry to see if it's severe. In my local testing, around 1 second between profile-do-change and the first getCookieString, and reading db sync takes only <10 ms (unless first time usage due to creating the table.)
Assignee | ||
Comment 46•7 years ago
|
||
> 2. Move [1] to sync read
> 3. Handle mDefaultDBState->readSet carefully
>
> [1]
> http://searchfox.org/mozilla-central/rev/
> 30a47c4339bd397b937abdb2305f99d3bb537ba6/netwerk/cookie/nsCookieService.
> cpp#2758-2772
Since
1) we abandon reading one domain in EnsureReadDomain (readSet),
2) we're not async reading anything (hostArray),
we need to remove readSet and hostArray.
Hence, no need to worry 2 and 3.
Assignee | ||
Comment 47•7 years ago
|
||
The patch works functionally. Still need some detail change. TODO: 1. I'm still thinking how to get the frequency we block the main thread. 2. HandleCorruptDB access the TryInitDB, which should be OMT 3. If I add a PR_Sleep after InitDBState, the gecko profiler will let the child process crash.
Attachment #8893710 -
Attachment is obsolete: true
Attachment #8897376 -
Flags: review?(hurley)
Assignee | ||
Comment 48•7 years ago
|
||
Comment on attachment 8897376 [details] [diff] [review] Part1_OMT_Init WIP1, v2 Should set a feedback flag. Sorry for spam.
Attachment #8897376 -
Flags: review?(hurley) → feedback?(hurley)
Comment 49•7 years ago
|
||
Comment on attachment 8897376 [details] [diff] [review] Part1_OMT_Init WIP1, v2 Review of attachment 8897376 [details] [diff] [review]: ----------------------------------------------------------------- This is looking good so far. It will be interesting to see what kind of telemetry we get out of this. ::: netwerk/base/nsIOService.cpp @@ +1479,5 @@ > nsCOMPtr<nsIPrefBranch> prefBranch; > GetPrefBranch(getter_AddRefs(prefBranch)); > PrefsChanged(prefBranch, MANAGE_OFFLINE_STATUS_PREF); > + > + // Bug 970460 - Read cookie database at an early-as-possible time nit: this is *8*70460, not 9... :) ::: netwerk/cookie/nsCookieService.cpp @@ +892,5 @@ > mDefaultDBState->removeListener = new RemoveCookieDBListener(mDefaultDBState); > mDefaultDBState->closeListener = new CloseCookieDBListener(mDefaultDBState); > > // Grow cookie db in 512KB increments > + mDefaultDBState->syncConn->SetGrowthIncrement(512 * 1024, EmptyCString()); We need to be sure this isn't a connection-specific setting before removing the call on dbConn, as well. @@ +2606,5 @@ > + while (!mInitialized) { > + mMonitor.Wait(); > + } > + // TODO: Need to send a telemetry > + // int64_t((TimeStamp::Now() - startBlockTime).ToMilliseconds()) Use Telemetry::AccumulateTimeDelta(<key>, startBlockTime); Also, we should either (1) accumulate a 0 if we never end up having to wait, or (2) have a separate telemetry key that tells whether or not we waited. Otherwise, we'll not get a sense of the ratio of times we have to wait to the times we never have to wait.
Attachment #8897376 -
Flags: feedback?(hurley) → feedback+
Comment 50•7 years ago
|
||
(In reply to Junior[:junior] from comment #47) > Created attachment 8897376 [details] [diff] [review] > Part1_OMT_Init WIP1, v2 > > The patch works functionally. Still need some detail change. > > TODO: > 1. I'm still thinking how to get the frequency we block the main thread. I think that option (1) above in my feedback comment is the right way here. You just need to make sure you only accumulate that once per session (so have a bool mAccumulatedWaitTelemetry or something, set that to true after accumulating, and only accumulate if it's false) > 2. HandleCorruptDB access the TryInitDB, which should be OMT Yes, this is important. We'll probably want to have separate telemetry for when we wait on this vs. when we wait on the initial read. > 3. If I add a PR_Sleep after InitDBState, the gecko profiler will let > the child process crash. That's... odd.
Assignee | ||
Comment 51•7 years ago
|
||
BlockMainThread for those in nsICookieManager and nsICookieService to see what happen https://treeherder.mozilla.org/#/jobs?repo=try&revision=d2a12e2e4e0e6fa1b4e087d9eff2c8d6c1470c65
Assignee | ||
Comment 52•7 years ago
|
||
> ::: netwerk/cookie/nsCookieService.cpp > @@ +892,5 @@ > > mDefaultDBState->removeListener = new RemoveCookieDBListener(mDefaultDBState); > > mDefaultDBState->closeListener = new CloseCookieDBListener(mDefaultDBState); > > > > // Grow cookie db in 512KB increments > > + mDefaultDBState->syncConn->SetGrowthIncrement(512 * 1024, EmptyCString()); > > We need to be sure this isn't a connection-specific setting before removing > the call on dbConn, as well. > Hello Marco, Sorry for a quick question. Do you know if |SetGrowthIncrement| is connection-specific? I guess it's sqlite file-based [1], but I'm not very sure. http://searchfox.org/mozilla-central/rev/5696c3e525fc8222674eed6a562f5fcbe804c4c7/storage/mozIStorageConnection.idl#229-243
Flags: needinfo?(mak77)
Comment 53•7 years ago
|
||
it's an option set at runtime on the vfs underlying a connection, and thus imo it is connection specific. In general it doesn't matter much because you usually have only 1 r/w connection and multiple read-only connections (since with WAL you can have concurrent readers, but you can never have concurrent writers, the writes would be serialized in both cases).
Flags: needinfo?(mak77)
Assignee | ||
Comment 54•7 years ago
|
||
(In reply to Marco Bonardo [::mak] from comment #53) > it's an option set at runtime on the vfs underlying a connection, and thus > imo it is connection specific. In general it doesn't matter much because you > usually have only 1 r/w connection and multiple read-only connections (since > with WAL you can have concurrent readers, but you can never have concurrent > writers, the writes would be serialized in both cases). Thanks. Therefore it seems great to move the SetGrowthIncrement to the r/w connection.
Assignee | ||
Comment 55•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c00693ddeded52878f1949b9e1ffd0d72d565172&selectedJob=125826483
Assignee | ||
Comment 56•7 years ago
|
||
We have two connections: syncConn(sync, cookie thread), dbConn(async, main thread) Every connection is restricted to one thread. Hence, the flow will be: -- Main Thread -- profile-do-change InitDBStates NS_GetSpecialDirectory <== have better in main thread since test mocked DirectorService with js code mThread->Dispatch -- Cookie Thread -- mMonitor lock mInitializedDBStates = false TryInitDB() <== start to do DB operations ReadAheadFile(mDefaultDBState->cookieFile) OpenUnsharedDatabase(syncConn) Read() mInitializedDBStates = true mMonitor release lock mMonitor.Notify DispatchToMainThread -- Main Thread -- InitDBConn OpenUnsharedDatabase(dbConn) <== should be after readAheadFile and on main thread
Assignee | ||
Comment 57•7 years ago
|
||
Still suffered from test-failures, most of them operate CookieService before cookie-db-read
Attachment #8897376 -
Attachment is obsolete: true
Assignee | ||
Comment 58•7 years ago
|
||
Short story about disable this test case: http://searchfox.org/mozilla-central/rev/999385a5e8c2d360cc37286882508075fc2078bd/extensions/cookie/test/unit/test_cookies_async_failure.js#210-276 reading cookies from a host (EnsureReadDomain) works from a corrupt DB in the first time Hence this test pass. We change our architecture to read whole db in the start-up, so lots change in this test
Assignee | ||
Comment 59•7 years ago
|
||
Hello Eric, We'd like to off-main-thread the cookie db start-up read. However, just found a main thread assertion in the TLDService http://searchfox.org/mozilla-central/rev/999385a5e8c2d360cc37286882508075fc2078bd/netwerk/dns/nsEffectiveTLDService.cpp#342 Any reasons for this constraint? Moreover, we really seldom use TLD service off-main-thread (<10000 users in a year encounter this I guess) http://searchfox.org/mozilla-central/rev/999385a5e8c2d360cc37286882508075fc2078bd/netwerk/cookie/nsCookieService.cpp#1035 Any workable (maybe not so fast) solution? Thanks!
Flags: needinfo?(erahm)
Assignee | ||
Comment 60•7 years ago
|
||
I feel it's a little fragile with new architecture If something we don't do good, main thread might be blocked forever. TODO: TLDService main thread issue.
Attachment #8902629 -
Attachment is obsolete: true
Attachment #8904510 -
Flags: feedback?(hurley)
Comment 61•7 years ago
|
||
(In reply to Junior[:junior] from comment #59) > Hello Eric, > We'd like to off-main-thread the cookie db start-up read. > However, just found a main thread assertion in the TLDService > http://searchfox.org/mozilla-central/rev/ > 999385a5e8c2d360cc37286882508075fc2078bd/netwerk/dns/nsEffectiveTLDService. > cpp#342 > > Any reasons for this constraint? Going to look at the patch soon, but really, a quick look at the code shows access to a data structure that isn't threadsafe. As such, it either needs locking, or single-threaded access. Whenever this was written, it looks like single-threaded access was assumed. Seems at first glance like we'll need to make the caching in the etld service threadsafe with a lock... (and perhaps the TLDCacheEntry struct, as well? Needs a deeper look than I can give right now.)
Comment 62•7 years ago
|
||
(In reply to Nicholas Hurley [:nwgh][:hurley] (also hurley@todesschaf.org) from comment #61) > (In reply to Junior[:junior] from comment #59) > > Hello Eric, > > We'd like to off-main-thread the cookie db start-up read. > > However, just found a main thread assertion in the TLDService > > http://searchfox.org/mozilla-central/rev/ > > 999385a5e8c2d360cc37286882508075fc2078bd/netwerk/dns/nsEffectiveTLDService. > > cpp#342 > > > > Any reasons for this constraint? > > Going to look at the patch soon, but really, a quick look at the code shows > access to a data structure that isn't threadsafe. As such, it either needs > locking, or single-threaded access. Whenever this was written, it looks like > single-threaded access was assumed. Seems at first glance like we'll need to > make the caching in the etld service threadsafe with a lock... (and perhaps > the TLDCacheEntry struct, as well? Needs a deeper look than I can give right > now.) Yes the problem is the outer cache is not thread-safe. We could make it thread-safe and reasonably efficient by using a read-write lock [1]. Please file a blocking bug and I can implement this (or you can do it yourself and I'm happy to review). [1] http://searchfox.org/mozilla-central/source/xpcom/threads/RWLock.h
Flags: needinfo?(erahm)
Comment 63•7 years ago
|
||
(In reply to Eric Rahm [:erahm] (please no mozreview requests) from comment #62) > (In reply to Nicholas Hurley [:nwgh][:hurley] (also hurley@todesschaf.org) > from comment #61) > > (In reply to Junior[:junior] from comment #59) > > > Hello Eric, > > > We'd like to off-main-thread the cookie db start-up read. > > > However, just found a main thread assertion in the TLDService > > > http://searchfox.org/mozilla-central/rev/ > > > 999385a5e8c2d360cc37286882508075fc2078bd/netwerk/dns/nsEffectiveTLDService. > > > cpp#342 > > > > > > Any reasons for this constraint? > > > > Going to look at the patch soon, but really, a quick look at the code shows > > access to a data structure that isn't threadsafe. As such, it either needs > > locking, or single-threaded access. Whenever this was written, it looks like > > single-threaded access was assumed. Seems at first glance like we'll need to > > make the caching in the etld service threadsafe with a lock... (and perhaps > > the TLDCacheEntry struct, as well? Needs a deeper look than I can give right > > now.) > > Yes the problem is the outer cache is not thread-safe. We could make it > thread-safe and reasonably efficient by using a read-write lock [1]. Please > file a blocking bug and I can implement this (or you can do it yourself and > I'm happy to review). > > [1] http://searchfox.org/mozilla-central/source/xpcom/threads/RWLock.h I'll take care of this today.
Comment 64•7 years ago
|
||
Comment on attachment 8904510 [details] [diff] [review] CookieDBStartupOMT - WIP, v4 Review of attachment 8904510 [details] [diff] [review]: ----------------------------------------------------------------- Looking pretty good! Looks like the etld service issue is about to be fixed, too. I am worried though... why are you concerned that we might block the main thread forever? ::: extensions/cookie/test/unit/xpcshell.ini @@ +29,5 @@ > [test_permmanager_idn.js] > [test_permmanager_subdomains.js] > [test_permmanager_local_files.js] > [test_permmanager_cleardata.js] > +#[test_schema_2_migration.js] Why is this commented out? ::: toolkit/components/telemetry/Histograms.json @@ +4407,5 @@ > + "MOZ_SQLITE_COOKIES_BLOCK_MAIN_THREAD_MS": { > + "record_in_processes": ["main"], > + "expires_in_version": "never", > + "kind": "exponential", > + "alert_emails": ["necko@mozilla.com", "bsmedberg@mozilla.com"], bsmedberg@mozilla is no longer a valid address. Just use necko@mozilla.com
Attachment #8904510 -
Flags: feedback?(hurley)
Assignee | ||
Comment 65•7 years ago
|
||
> > Looking pretty good! Looks like the etld service issue is about to be fixed, > too. > > I am worried though... why are you concerned that we might block the main > thread forever? > When I wrote the patch, mMonitor.Wait() is not notified for many cases I was aware by test case. The flow control is complicated. We have mInitializedDBStates and mInitializaedDBConn for controlling the status. Also have (!mDBStates) to determine if the DBStates is closed or not. I'm afraid if there's some corner flaw I'm not aware. > ::: extensions/cookie/test/unit/xpcshell.ini > @@ +29,5 @@ > > [test_permmanager_idn.js] > > [test_permmanager_subdomains.js] > > [test_permmanager_local_files.js] > > [test_permmanager_cleardata.js] > > +#[test_schema_2_migration.js] > > Why is this commented out? > Since it's blocked by bug 1396958. Will remove it after it lands. > ::: toolkit/components/telemetry/Histograms.json > @@ +4407,5 @@ > > + "MOZ_SQLITE_COOKIES_BLOCK_MAIN_THREAD_MS": { > > + "record_in_processes": ["main"], > > + "expires_in_version": "never", > > + "kind": "exponential", > > + "alert_emails": ["necko@mozilla.com", "bsmedberg@mozilla.com"], > > bsmedberg@mozilla is no longer a valid address. Just use necko@mozilla.com Thanks for your remind. Do you have a suggested candidate for data review?
Flags: needinfo?(hurley)
Assignee | ||
Comment 66•7 years ago
|
||
I'm thinking we could ignore the old cookie format. Simply create a new database for them, since the cookies should be stale. It could save some CPU times and code complexity For example, we migrate from .txt to .sqlite 9 years ago but we still able to import .txt Everytime we create a new table causing this check. bug: https://bugzilla.mozilla.org/show_bug.cgi?id=230933 changeset: https://github.com/mozilla/gecko-dev/commit/5493e24575253cdf46b42878d33d1262e4f82eae back-compat code http://searchfox.org/mozilla-central/rev/4d8e389498a08668cce9ebf6232cc96be178c3e4/netwerk/cookie/nsCookieService.cpp#1518-1531 Another example is a migration from schema 1 to 2. bug: https://bugzilla.mozilla.org/show_bug.cgi?id=599799 In each startup, we're doing a no-op for >99% case to delete a non-exist null baseDomain record. http://searchfox.org/mozilla-central/rev/4d8e389498a08668cce9ebf6232cc96be178c3e4/netwerk/cookie/nsCookieService.cpp#2682-2685
Assignee | ||
Comment 67•7 years ago
|
||
Hello :francois, We off-main-thread the startup cookie read. If anyone access to the cookie during the OMT read, we simply block main thread (it's should be rare) Here we put a probe to see if it's rare or not.
Attachment #8904510 -
Attachment is obsolete: true
Flags: needinfo?(hurley)
Attachment #8904905 -
Flags: review?(hurley)
Attachment #8904905 -
Flags: feedback?(francois)
Comment 68•7 years ago
|
||
Comment on attachment 8904905 [details] [diff] [review] CookieDBStartupOMT, v5 Review of attachment 8904905 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/telemetry/Histograms.json @@ +4407,5 @@ > + "MOZ_SQLITE_COOKIES_BLOCK_MAIN_THREAD_MS": { > + "record_in_processes": ["main"], > + "expires_in_version": "never", > + "kind": "exponential", > + "alert_emails": ["necko@mozilla.com", "francois@mozilla.com"], I don't want these alerts :) I think you meant to use your own email address here.
Attachment #8904905 -
Flags: feedback?(francois) → feedback-
Assignee | ||
Comment 69•7 years ago
|
||
>
> I don't want these alerts :)
>
> I think you meant to use your own email address here.
Will remove in the next patch :)
Comment 70•7 years ago
|
||
Comment on attachment 8904905 [details] [diff] [review] CookieDBStartupOMT, v5 Review of attachment 8904905 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, but I do have one worry. See below. ::: netwerk/cookie/nsCookie.h @@ +30,5 @@ > class nsCookie : public nsICookie2 > { > public: > // nsISupports > + NS_DECL_THREADSAFE_ISUPPORTS Do we know this is safe? Generally when we've made something non-threadsafe (especially something this old), it's for a purpose. Based on a quick look, I'm not so sure that all the uses of the members are threadsafe. ::: netwerk/cookie/nsCookieService.cpp @@ +1383,5 @@ > } > > + // if we deleted a corrupt db, don't attempt to import - return now > + if (aRecreateDB) > + return RESULT_OK; braces around body, please. @@ +1401,5 @@ > + ); > + return RESULT_OK; > + } > + > + nsCOMPtr<nsIRunnable> runnable = nit: trailing whitespace on this line. @@ +1406,5 @@ > + NS_NewRunnableFunction("TryInitDB.ImportCookies", [self] { > + nsCOMPtr<nsIFile> oldCookieFile; > + nsresult rv = NS_GetSpecialDirectory(NS_APP_USER_PROFILE_50_DIR, > + getter_AddRefs(oldCookieFile)); > + if (NS_FAILED(rv)) return; braces around body, and body on its own line, please. ::: toolkit/components/telemetry/Histograms.json @@ +4407,5 @@ > + "MOZ_SQLITE_COOKIES_BLOCK_MAIN_THREAD_MS": { > + "record_in_processes": ["main"], > + "expires_in_version": "never", > + "kind": "exponential", > + "alert_emails": ["necko@mozilla.com", "francois@mozilla.com"], Either what Francois said, or just leave it as necko@mozilla.com without emails direct to you.
Attachment #8904905 -
Flags: review?(hurley)
Comment 71•7 years ago
|
||
(In reply to Nicholas Hurley [:nwgh][:hurley] (also hurley@todesschaf.org) from comment #70) > Either what Francois said, or just leave it as necko@mozilla.com without > emails direct to you. Having necko in there is fine, but we also need the email of a person responsible for this probe. Most often an engineer, sometimes a PM.
Assignee | ||
Comment 72•7 years ago
|
||
> ::: netwerk/cookie/nsCookie.h
> @@ +30,5 @@
> > class nsCookie : public nsICookie2
> > {
> > public:
> > // nsISupports
> > + NS_DECL_THREADSAFE_ISUPPORTS
>
> Do we know this is safe? Generally when we've made something non-threadsafe
> (especially something this old), it's for a purpose. Based on a quick look,
> I'm not so sure that all the uses of the members are threadsafe.
>
Thanks for catching this.
With respect to the usage of nsCookie, two things happen to cookie thread:
1) InitDBStates, protect by mMonitor and mInitializedDBStates.
2) RebuildCorruptDB, only "read" the hash table
RebuildCurruptDB is a little dangerous since there's a little chance that
someone writing cookie when we're rebuilding.
Rebuilding is rare, so mutex doesn't make sense.
I'm thinking a deep-copy of the cookie hash table on the main thread and pass
it to rebuild. What do you think?
Flags: needinfo?(hurley)
Assignee | ||
Comment 73•7 years ago
|
||
(In reply to Junior[:junior] from comment #72) > > ::: netwerk/cookie/nsCookie.h > > @@ +30,5 @@ > > > class nsCookie : public nsICookie2 > > > { > > > public: > > > // nsISupports > > > + NS_DECL_THREADSAFE_ISUPPORTS > > > > Do we know this is safe? Generally when we've made something non-threadsafe > > (especially something this old), it's for a purpose. Based on a quick look, > > I'm not so sure that all the uses of the members are threadsafe. > > > > Thanks for catching this. > > With respect to the usage of nsCookie, two things happen to cookie thread: > 1) InitDBStates, protect by mMonitor and mInitializedDBStates. > 2) RebuildCorruptDB, only "read" the hash table > > RebuildCurruptDB is a little dangerous since there's a little chance that > someone writing cookie when we're rebuilding. > > Rebuilding is rare, so mutex doesn't make sense. > > I'm thinking a deep-copy of the cookie hash table on the main thread and pass > it to rebuild. What do you think? I found that we don't do many things for rebuilding in cookie thread. |TryInitDB(false)| recreates a brand-new db without touching hosttable, which should be taken care. And read the hosttable in the main thread.
Flags: needinfo?(hurley)
Assignee | ||
Comment 74•7 years ago
|
||
Attachment #8904905 -
Attachment is obsolete: true
Attachment #8906509 -
Flags: review?(hurley)
Comment 75•7 years ago
|
||
Comment on attachment 8906509 [details] [diff] [review] CookieDBStartupOMT, v6 Review of attachment 8906509 [details] [diff] [review]: ----------------------------------------------------------------- ::: netwerk/cookie/nsCookie.h @@ +30,5 @@ > class nsCookie : public nsICookie2 > { > public: > // nsISupports > + NS_DECL_THREADSAFE_ISUPPORTS This is still not safe. While you may have ensured the uses of it *in nsCookieService* are now threadsafe (I haven't looked that far in the patch), there are other consumers of this interface that could do something wrong. If you are making the refcounting threadsafe, you need to ensure its other data members are threadsafe too, otherwise you're just introducing a footgun that's basically guaranteed to go off at some point, likely resulting in a chemspill.
Attachment #8906509 -
Flags: review?(hurley) → review-
Assignee | ||
Comment 76•7 years ago
|
||
(In reply to Nicholas Hurley [:nwgh][:hurley] (also hurley@todesschaf.org) from comment #75) > Comment on attachment 8906509 [details] [diff] [review] > CookieDBStartupOMT, v6 > > Review of attachment 8906509 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: netwerk/cookie/nsCookie.h > @@ +30,5 @@ > > class nsCookie : public nsICookie2 > > { > > public: > > // nsISupports > > + NS_DECL_THREADSAFE_ISUPPORTS > > This is still not safe. While you may have ensured the uses of it *in > nsCookieService* are now threadsafe (I haven't looked that far in the > patch), there are other consumers of this interface that could do something > wrong. If you are making the refcounting threadsafe, you need to ensure its > other data members are threadsafe too, otherwise you're just introducing a > footgun that's basically guaranteed to go off at some point, likely > resulting in a chemspill. Let's try this: After we finish database reads, we pass the "keys" (parameters of nsCookie::Create) to main thread, and let main thread convert those to nsCookie.
Comment 77•7 years ago
|
||
Bulk priority update: https://bugzilla.mozilla.org/show_bug.cgi?id=1399258
Priority: -- → P1
Comment 78•7 years ago
|
||
Changing to P2 as this will not be landed in 57, and we would like to land it in the next iteration.
Priority: P1 → P2
Whiteboard: [qf:p1][necko-quantum] [necko-active] → [qf:p1][necko-quantum]
Comment 79•7 years ago
|
||
Based on #comment78, I am moving this from qf:p1 to qf:p2.
Whiteboard: [qf:p1][necko-quantum] → [qf:p2][necko-quantum]
Assignee | ||
Comment 80•7 years ago
|
||
Introduce a ConstCookie for threadsafe nsCookie generator. And use UniquePtr to avoid deep copy.
Attachment #8906509 -
Attachment is obsolete: true
Attachment #8910694 -
Flags: review?(hurley)
Comment 81•7 years ago
|
||
Comment on attachment 8910694 [details] [diff] [review] CookieDBStartupOMT, v7 Review of attachment 8910694 [details] [diff] [review]: ----------------------------------------------------------------- Nice progress, but one other thing I noticed this time around. ::: netwerk/cookie/nsCookieService.h @@ +152,5 @@ > { > } > > public: > + NS_INLINE_DECL_THREADSAFE_REFCOUNTING(DBState) I just went back and looked and saw this was in the previous version, too. Sorry I didn't catch it that time. Anyway, this is yet another case of making something threadsafe without ensuring its members (specifically the hash table) are not accessed in a manner inconsistent with that assumption.
Attachment #8910694 -
Flags: review?(hurley)
Assignee | ||
Comment 82•7 years ago
|
||
Use gCookieService instead of adding refcount
Attachment #8910694 -
Attachment is obsolete: true
Attachment #8911686 -
Flags: review?(hurley)
Comment 83•7 years ago
|
||
Comment on attachment 8911686 [details] [diff] [review] CookieDBStartupOMT, v8 Review of attachment 8911686 [details] [diff] [review]: ----------------------------------------------------------------- I'm still not convinced here. Now, you're trading refcounting on separate threads (which actually might be the right idea, you just need to make sure it's used safely) for using (effectively) a raw pointer on multiple threads, without even null-checking it. Again, it may be the case that this is safe in practice, as the code is written, but there's no guarantees of that in the code. That's a recipe for crashes and/or data races that we don't want. Since you're going to be accessing the cookie service on multiple threads, you need to have some sort of protection around the things that are accessed on multiple threads (mDBState comes to mind, at least, though there are almost certainly more).
Attachment #8911686 -
Flags: review?(hurley) → review-
Assignee | ||
Comment 84•7 years ago
|
||
Add some guards. Suffer from a treeherder only fail now. Might be a test problem, however stderr doesn't work in RunnableFunction lambda . Finding a way to debug
Attachment #8911686 -
Attachment is obsolete: true
Attachment #8912181 -
Flags: review?(hurley)
Assignee | ||
Comment 85•7 years ago
|
||
> Might be a test problem, however stderr doesn't work in RunnableFunction
> lambda . Finding a way to debug
Neither an implementation of nsIRunnable
Assignee | ||
Comment 86•7 years ago
|
||
(In reply to Junior[:junior] from comment #85) > > Might be a test problem, however stderr doesn't work in RunnableFunction > > lambda . Finding a way to debug > Neither an implementation of nsIRunnable stderr works in treeherder, which is good
Assignee | ||
Comment 87•7 years ago
|
||
Ensure the task is sent after we set the mInitializedDBStates. Also fix potential-intermittent tests. Rebase from Bug 1286858.
Attachment #8912181 -
Attachment is obsolete: true
Attachment #8912181 -
Flags: review?(hurley)
Attachment #8912685 -
Flags: review?(hurley)
Assignee | ||
Comment 88•7 years ago
|
||
Note that syncConn is able to accessed by cookie thread, same as RELEASE refcount. Hence we remove the null-out in main thread.
Attachment #8913110 -
Flags: review?(hurley)
Assignee | ||
Updated•7 years ago
|
Attachment #8912685 -
Attachment description: CookieDBStartupOMT, v10 → Part1: CookieDBStartupOMT, v10
Comment 89•7 years ago
|
||
Comment on attachment 8912685 [details] [diff] [review] Part1: CookieDBStartupOMT, v10 Review of attachment 8912685 [details] [diff] [review]: ----------------------------------------------------------------- So... I think this is probably ok, the more I look at it (I wasn't convinced at first). But I want to get a second set of eyes on this, as well, to make sure I'm not just going wonky-eyed from looking at this patch for too long :) So, r+ from me, but let's ask someone else who's familiar with the code. jdm, do you have time to take a look?
Attachment #8912685 -
Flags: review?(josh)
Attachment #8912685 -
Flags: review?(hurley)
Attachment #8912685 -
Flags: review+
Attachment #8913110 -
Flags: review?(hurley) → review+
Reporter | ||
Updated•7 years ago
|
Whiteboard: [qf:p2][necko-quantum] → [qf:p2][necko-quantum][necko-triaged]
Comment 90•7 years ago
|
||
Comment on attachment 8912685 [details] [diff] [review] Part1: CookieDBStartupOMT, v10 Review of attachment 8912685 [details] [diff] [review]: ----------------------------------------------------------------- This is a clever solution, and I can't think of any way to make how it all fits together more clear. ::: netwerk/cookie/nsCookieService.cpp @@ +643,5 @@ > + nsCOMPtr<nsIThread> thread; > + rv = NS_NewNamedThread("Cookie", getter_AddRefs(thread)); > + NS_ENSURE_SUCCESS(rv, rv); > + > + mThread = thread; Can we use `NS_NewNamedThread("Cookie", getter_AddRefs(mThread))` instead?
Attachment #8912685 -
Flags: review?(josh) → review+
Assignee | ||
Comment 91•7 years ago
|
||
Need a data review. Comment 67 had the description of the telemetry usage.
Attachment #8912685 -
Attachment is obsolete: true
Attachment #8916517 -
Flags: review+
Attachment #8916517 -
Flags: feedback?(francois)
Assignee | ||
Comment 92•7 years ago
|
||
rebase, carry r+
Attachment #8913110 -
Attachment is obsolete: true
Attachment #8916518 -
Flags: review+
Comment 93•7 years ago
|
||
Comment on attachment 8916517 [details] [diff] [review] Part1: CookieDBStartupOMT, v11 Review of attachment 8916517 [details] [diff] [review]: ----------------------------------------------------------------- This is Category 1 data. datareview+
Attachment #8916517 -
Flags: feedback?(francois) → feedback+
Assignee | ||
Comment 94•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a05a8fceb4962b38cb915eb2c5f6c00671cf1ca8
Keywords: checkin-needed
Assignee | ||
Comment 95•7 years ago
|
||
I had some experiment at reference device (Acer). Results: a. Performance Improvement Normal case: 6.8ms [1] -> 3.2ms [2] Corrupted case: 245ms [3] -> 24ms [4] b. Main thread blocking problem We finished the init sequence at 0.5-0.6 sec, and the first access of cookieService (getCookieStringInternal) is at 1.3s Explanations: We have two connections for cookie db access. We OMT the sync startup one, so we have around 50% off for normal case. OTOH corrupted DB janks heavily (comment 28) and brings this patch into full play. We OMT 363ms to cookie thread in another experiment [4] (I remove the cookie.sqlite manually) Still cost 24ms on main thread to import cookie.txt, which I tent to remove (comment 66). Again, 0.7s between initialization and the first getCookieStringInternal here, too. [1] https://perfht.ml/2xYIgfb [2] https://perfht.ml/2xYQSlS [3] https://perfht.ml/2xYj48j [4] https://perfht.ml/2xZOlba
Comment 96•7 years ago
|
||
Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/5aa7980268cb Part 1: Let cookie db startup-read off-main-thread. r=nwgh, r=jdm, data-r=francois https://hg.mozilla.org/integration/mozilla-inbound/rev/50368fa3e040 Part 2: Close syncconn for edge cases. r=nwgh
Keywords: checkin-needed
Comment 97•7 years ago
|
||
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/ede558a89106 for xperf failures, https://treeherder.mozilla.org/logviewer.html#?job_id=136291092&repo=mozilla-inbound
![]() |
||
Updated•7 years ago
|
Assignee | ||
Comment 98•7 years ago
|
||
Hello :jmaher, Previously we use a lazy load of cookie service, so startup won't have any IO on cookies.sqlite and cookies.sqlite-shm. Here we intentionally load the cookie service at startup and make db loading off-main-thread to earn some main thread performance win. comment 21 for a more detailed description. Hence, we should put the db files into xperf whitelist.
Attachment #8917667 -
Flags: review?(jmaher)
Comment 99•7 years ago
|
||
Comment on attachment 8917667 [details] [diff] [review] Part3: talos-whitelist, v1 Review of attachment 8917667 [details] [diff] [review]: ----------------------------------------------------------------- thanks for updating this. As a note, did you test on pgo? Often we get different file access patterns on PGO vs OPT.
Attachment #8917667 -
Flags: review?(jmaher) → review+
Assignee | ||
Comment 100•7 years ago
|
||
(In reply to Joel Maher ( :jmaher) (UTC-5) from comment #99) > Comment on attachment 8917667 [details] [diff] [review] > Part3: talos-whitelist, v1 > > Review of attachment 8917667 [details] [diff] [review]: > ----------------------------------------------------------------- > > thanks for updating this. As a note, did you test on pgo? Often we get > different file access patterns on PGO vs OPT. Thanks for reviewing and reminding the PGO. Let's wait for the result. https://treeherder.mozilla.org/#/jobs?repo=try&revision=79d30898f16f15ef5e4f2a5f50209ca6fd10e8d6&group_state=expanded&filter-searchStr=talos
Comment 102•7 years ago
|
||
Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/fe37f3401424 Part 1: Let cookie db startup-read off-main-thread. r=nwgh, r=jdm, data-r=francois https://hg.mozilla.org/integration/mozilla-inbound/rev/d76e2193be25 Part 2: Close syncconn for edge cases. r=nwgh https://hg.mozilla.org/integration/mozilla-inbound/rev/5d316717a69a Part 3: Add cookies.sqlite{|-shm} to xperf whitelist. r=jmaher
Keywords: checkin-needed
![]() |
||
Comment 103•7 years ago
|
||
Backed out for frequently failing chrome tests on Windows 10: Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=5d316717a69a8bbc6892ac97c415d79c463d2673&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel Failure log example, please also check other failures: https://treeherder.mozilla.org/logviewer.html#?job_id=136543672&repo=mozilla-inbound > TEST-UNEXPECTED-FAIL | leakcheck | default process: 176 bytes leaked (Mutex, nsBaseAppShell) The issue could also be observed on the previous landing: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&tochange=ede558a891066160cef9ec410526b64e33537c22&fromchange=0bf9f589e2428cd6fdd49fea0c93632870ec2cb4
Flags: needinfo?(juhsu)
![]() |
||
Comment 104•7 years ago
|
||
Backout changesets: https://hg.mozilla.org/integration/mozilla-inbound/rev/2a0d5d48504e002a01cfa3a4b982e435bc2e1115 https://hg.mozilla.org/integration/mozilla-inbound/rev/f0116fc2f100be36942d4826a46e2e15c9fc113d https://hg.mozilla.org/integration/mozilla-inbound/rev/e47739050bc406c4e66c46efb50227493e3d395b
Comment 105•7 years ago
|
||
When this bug landed, these performance regressions were spotted. They are serious, so please take them into account. == Change summary for alert #9984 (as of October 12 2017 14:56 UTC) == Regressions: 19% tp5n nonmain_startup_fileio windows7-32 pgo e10s 2,021,375.67 -> 2,404,610.00 6% tp5n nonmain_startup_fileio windows7-32 opt e10s 2,034,222.38 -> 2,149,956.09 For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=9984
Comment 106•7 years ago
|
||
The backout canceled the regressions: == Change summary for alert #9991 (as of October 12 2017 18:32 UTC) == Improvements: 16% tp5n nonmain_startup_fileio windows7-32 pgo e10s 2,402,032.33 -> 2,021,094.25 1% tp5n nonmain_startup_fileio windows7-32 opt e10s 2,080,908.83 -> 2,050,527.25 For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=9991
Assignee | ||
Comment 107•7 years ago
|
||
(In reply to IonuΘ Goldan [:igoldan], Performance Sheriffing from comment #106) > The backout canceled the regressions: > > == Change summary for alert #9991 (as of October 12 2017 18:32 UTC) == > > Improvements: > > 16% tp5n nonmain_startup_fileio windows7-32 pgo e10s 2,402,032.33 -> > 2,021,094.25 > 1% tp5n nonmain_startup_fileio windows7-32 opt e10s 2,080,908.83 -> > 2,050,527.25 > > For up to date results, see: > https://treeherder.mozilla.org/perf.html#/alerts?id=9991 Thanks for your notice. As comment 98 said, we move the io earlier. (Before this patch, we load the cookie file when we access the cookie first time. Usually we open browser for browsing a web, hence the io is almost necessary.)
Flags: needinfo?(juhsu)
Assignee | ||
Comment 108•7 years ago
|
||
(In reply to Sebastian Hengst [:aryx][:archaeopteryx] (needinfo on intermittent or backout) from comment #103) > Backed out for frequently failing chrome tests on Windows 10: > > Push with failures: > https://treeherder.mozilla.org/#/jobs?repo=mozilla- > inbound&revision=5d316717a69a8bbc6892ac97c415d79c463d2673&filter- > resultStatus=testfailed&filter-resultStatus=busted&filter- > resultStatus=exception&filter-resultStatus=retry&filter- > resultStatus=usercancel > Failure log example, please also check other failures: > https://treeherder.mozilla.org/logviewer.html#?job_id=136543672&repo=mozilla- > inbound I've re-triggered other failures and taken a look. Other failures all passed and I think they are un-related to cookie. On the other hand, I have no idea about the Windows 10 leakages. Per bug 1401883 comment 5, nsBaseAppShell is not released for some reason. And my patch unveils this leakage frequently. I have some random try on treeherder and wait for the result now. > > TEST-UNEXPECTED-FAIL | leakcheck | default process: 176 bytes leaked (Mutex, nsBaseAppShell) > > The issue could also be observed on the previous landing: > https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&filter- > resultStatus=testfailed&filter-resultStatus=busted&filter- > resultStatus=exception&tochange=ede558a891066160cef9ec410526b64e33537c22&from > change=0bf9f589e2428cd6fdd49fea0c93632870ec2cb4
Assignee | ||
Comment 109•7 years ago
|
||
Should be a thread life-cycle issue. It works to replace nsIThread to StreamTransportService.
Assignee | ||
Comment 110•7 years ago
|
||
I believe changing threading life-cycle should work. https://treeherder.mozilla.org/#/jobs?repo=try&revision=b6963a183d2032a456d6f58f031251e89601f5d0
Assignee | ||
Comment 111•7 years ago
|
||
Comment on attachment 8919205 [details] [diff] [review] Part4: threadLifecycle - v1 treeherder shows the intermittent rate is decreased from 30% to <10%.
Attachment #8919205 -
Flags: review?(hurley)
Assignee | ||
Comment 112•7 years ago
|
||
(In reply to Junior[:junior] from comment #111) > Comment on attachment 8919205 [details] [diff] [review] > Part4: threadLifecycle - v1 > > treeherder shows the intermittent rate is decreased from 30% to <10%. As Comment 109 said, if I use a short life-cycle thread (StreamTransportService). The intermittent rate is down to <5% https://treeherder.mozilla.org/#/jobs?repo=try&revision=0bfd99ad67846e6b54f24ad1f6107d0a03176063 If we only omt startup and rebuild, it's fine to use StreamTransportService.
Assignee | ||
Comment 113•7 years ago
|
||
[1] indicates that we should Shutdown at event "xpcom-shutdown-threads" Shutdown sequence would be: profile-before-change -> xpcom-shutdown-threads -> xpcom-shutdown FWIW, before Part4, cookie service is released during nsCycleCollector_shutdown, later than xpocm-shutdown. [1] http://searchfox.org/mozilla-central/rev/dca019c94bf3a840ed7ff50261483410cfece24f/xpcom/build/XPCOMInit.cpp#805
Comment 115•7 years ago
|
||
Sorry, this got lost in the shuffle last week. I'm reviewing today.
Flags: needinfo?(hurley)
Comment 116•7 years ago
|
||
Comment on attachment 8919205 [details] [diff] [review] Part4: threadLifecycle - v1 Review of attachment 8919205 [details] [diff] [review]: ----------------------------------------------------------------- LGTM. Seems like the leak is a pre-existing condition, and these patches just made it more common, right? And with this patch it returns to its previous level of intermittent-ness?
Attachment #8919205 -
Flags: review?(hurley) → review+
Assignee | ||
Comment 117•7 years ago
|
||
(In reply to Nicholas Hurley [:nwgh][:hurley] (also hurley@todesschaf.org) from comment #116) > Comment on attachment 8919205 [details] [diff] [review] > Part4: threadLifecycle - v1 > > Review of attachment 8919205 [details] [diff] [review]: > ----------------------------------------------------------------- > > LGTM. Seems like the leak is a pre-existing condition, and these patches > just made it more common, right? And with this patch it returns to its > previous level of intermittent-ness? As far as I can tell, 1) yes we are supposed to shutdown the thread before xpcom-shutdown-thread. 2) The intermittent rate decreases a lot. With some confidence, it goes back to previous level.
Assignee | ||
Updated•7 years ago
|
Keywords: checkin-needed
Comment 118•7 years ago
|
||
Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/6c4818576443 Part 1: Let cookie db startup-read off-main-thread. r=nwgh, r=jdm, data-r=francois https://hg.mozilla.org/integration/mozilla-inbound/rev/b0ddf460db07 Part 2: Close syncconn for edge cases. r=nwgh https://hg.mozilla.org/integration/mozilla-inbound/rev/224e86671743 Part 3: Add cookies.sqlite{|-shm} to xperf whitelist. r=jmaher https://hg.mozilla.org/integration/mozilla-inbound/rev/edacb0144b17 Part 4: Make the lifecycle of cookie thread alongwith the profile. r=ngwh
Keywords: checkin-needed
![]() |
||
Comment 119•7 years ago
|
||
Backed out for failing many xpcshell tests, e.g. extensions/cookie/test/unit/test_cookies_async_failure.js: https://hg.mozilla.org/integration/mozilla-inbound/rev/69d0650dfbe945e0b44d28efafdd483d1ef4e738 https://hg.mozilla.org/integration/mozilla-inbound/rev/df2b4f5744e320ede6a31985c008dbf21195338d https://hg.mozilla.org/integration/mozilla-inbound/rev/4d652fad1384c147b6e904a7644dff7f323d5067 https://hg.mozilla.org/integration/mozilla-inbound/rev/c560488dbd1157aa1f143e12c4c488775f69269c Push with failures (also check the TV jobs): https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=edacb0144b171382406a69e1b74cd9a4df83b9fd&filter-resultStatus=usercancel&filter-resultStatus=runnable&filter-resultStatus=retry&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=139182828&repo=mozilla-inbound 07:07:35 INFO - TEST-START | extensions/cookie/test/unit/test_cookies_async_failure.js 07:12:35 WARNING - TEST-UNEXPECTED-TIMEOUT | extensions/cookie/test/unit/test_cookies_async_failure.js | Test timed out 07:12:35 INFO - TEST-INFO took 300000ms 07:12:35 INFO - >>>>>>> 07:12:35 INFO - (xpcshell/head.js) | test MAIN run_test pending (1) 07:12:35 INFO - (xpcshell/head.js) | test pending (2) 07:12:35 INFO - TEST-PASS | extensions/cookie/test/unit/test_cookies_async_failure.js | do_run_test - [do_run_test : 43] true == true 07:12:35 INFO - TEST-PASS | extensions/cookie/test/unit/test_cookies_async_failure.js | do_run_test - [do_run_test : 44] true == true 07:12:35 INFO - (xpcshell/head.js) | test MAIN run_test finished (2) 07:12:35 INFO - running event loop 07:12:35 INFO - "CONSOLE_MESSAGE: (info) No chrome package registered for chrome://branding/locale/brand.properties" 07:12:35 INFO - TEST-PASS | extensions/cookie/test/unit/test_cookies_async_failure.js | observe - [observe : 67] "cookie-db-closed" == "cookie-db-closed" 07:12:35 INFO - TEST-PASS | extensions/cookie/test/unit/test_cookies_async_failure.js | run_test_1 - [run_test_1 : 140] 2 == 2 07:12:35 INFO - <<<<<<<
Flags: needinfo?(juhsu)
Assignee | ||
Comment 120•7 years ago
|
||
Since nsThread::Shutdown would execute all the tasks dispatched to main thread, we should do those task before closing everything. https://treeherder.mozilla.org/#/jobs?repo=try&revision=febe2b5fccf196006e58ca12ce5ad5590f46cfbc
Attachment #8919205 -
Attachment is obsolete: true
Flags: needinfo?(juhsu)
Attachment #8921791 -
Flags: review+
Assignee | ||
Updated•7 years ago
|
Keywords: checkin-needed
Comment 121•7 years ago
|
||
Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/1d83d62684e2 Part 1: Let cookie db startup-read off-main-thread. r=nwgh, r=jdm, data-r=francois https://hg.mozilla.org/integration/mozilla-inbound/rev/17f26189df9a Part 2: Close syncconn for edge cases. r=nwgh https://hg.mozilla.org/integration/mozilla-inbound/rev/3e5213e725a2 Part 3: Add cookies.sqlite{|-shm} to xperf whitelist. r=jmaher https://hg.mozilla.org/integration/mozilla-inbound/rev/3531caed0bd9 Part 4: Make the lifecycle of cookie thread alongwith the profile. r=ngwh
Keywords: checkin-needed
![]() |
||
Comment 122•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/1d83d62684e2 https://hg.mozilla.org/mozilla-central/rev/17f26189df9a https://hg.mozilla.org/mozilla-central/rev/3e5213e725a2 https://hg.mozilla.org/mozilla-central/rev/3531caed0bd9
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox58:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Assignee | ||
Updated•7 years ago
|
Updated•3 years ago
|
Performance Impact: --- → P2
Whiteboard: [qf:p2][necko-quantum][necko-triaged] → [necko-quantum][necko-triaged]
You need to log in
before you can comment on or make changes to this bug.
Description
•