Closed Bug 763555 Opened 8 years ago Closed 4 years ago

Do not do I/O while holding the cache lock

Categories

(Core :: Networking: Cache, defect)

defect
Not set

Tracking

()

RESOLVED WONTFIX

People

(Reporter: u408661, Assigned: michal)

References

Details

(Whiteboard: [tech-bug])

Attachments

(1 file)

Right now, hold the cache lock while we're doing I/O for opening, closing, and reading metadat in the cache (pehaps other times). We should release the lock when we're doing this so as to not block other threads that may be waiting on the lock.
Assignee: nobody → hurley
Assignee: hurley → michal.novotny
Blocks: 828997
No longer blocks: 828997
Blocks: 828997
(One of the sources of b2g jank that we prefer not to talk about.)
Blocks: b2g-v-next
We don't like to talk about this.
Whiteboard: [tech-bug]
Michal - when you have a patch up can you make sure to get feedback on it from Taras and :vladan?
There are too many possible call stacks ending up doing IO in nsDiskCacheMap::ReadDiskCacheEntry(). This patch reduces lot of them by replacing call to ProcessPendingRequests() by async call. Try server https://tbpl.mozilla.org/?tree=Try&rev=6d07fd5e58d5 is not green, but most of the failures seem to be unrelated to this patch. Asking for feedback instead of review because of xpcshell failures that still need to be resolved.
Attachment #722487 - Flags: feedback?(hurley)
Comment on attachment 722487 [details] [diff] [review]
replace calls to nsCacheService::ProcessPendingRequests() with async call when possible

Cancelling feedback based on the new plans.
Attachment #722487 - Flags: feedback?(hurley)
The biggest source of jank is actually from SetExpirationTime and is one of biggest chromehang issue after Plugins/GG/CC:

http://mxr.mozilla.org/mozilla-central/source/netwerk/cache/nsCacheEntryDescriptor.cpp#200

This means we will wait for an I/O to complete in another thread just to update an integer. Can't we just mitigate this problem by doing PostTask to the cache thread?
Question is: can nsCacheEntryDescriptor.mCacheEntry be nullified (by call to ClearCacheEntry()) while the descriptor is still referenced?  I assume it can.  Then we cannot check or in any way call on mCacheEntry while not holding the global lock.

This will of course not be the case in the new design ;)
Sorry:

(In reply to Honza Bambas (:mayhemer) from comment #7)
> I assume it can *NOT*.
does not apply to new cache
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.