The Open and Read operation of the CacheFileIOManager don't need to call back to the invoking thread. Calling back on the IO thread speeds up opening of existing cache entries. It's possible because of the well made thread safety of the surrounding upper levels of the code. The performance data has been collected via talos runs: cache1 vs cache2 w/o the optimization: http://compare-talos.mattn.ca/?oldRevs=4d3b22782bf1&newRev=1f8386ff9889&server=graphs.mozilla.org&submit=true cache1 vs cache2 w/ the optimization: http://compare-talos.mattn.ca/?oldRevs=4d3b22782bf1&newRev=65c3ed06cefb&server=graphs.mozilla.org&submit=true The win here is visible on tp5o_paint subtests, mainly on myspace.com that significantly regresses with cache2 w/o this optimization. (bug 982598 comment 16)
Michal, should I make this optional or is it OK to always to call the result directly all the time?
complete run: https://tbpl.mozilla.org/?tree=Try&rev=90b1635eb995
Doesn't help with bug 993649
It is OK to call listeners in CacheFileMetadata, CacheIndex, CacheFileChunk and CacheFile on IO thread. They will then call further listeners also on IO thread and I don't know whether it is always safe. Anyway, please make this optional so that we can still notify listeners on the original thread if needed.
- optional arg to OpenFile and Read to deliver the result on the IO thread - only locally tested since trees are closed :/
Assignee: nobody → honzab.moz
Status: NEW → ASSIGNED
Attachment #8410971 - Flags: review?(michal.novotny)
Comment on attachment 8410971 [details] [diff] [review] v1 Review of attachment 8410971 [details] [diff] [review]: ----------------------------------------------------------------- r+ with the following fixed ::: netwerk/cache2/CacheFileIOManager.h @@ +237,5 @@ > // Shuts the scheduling off and flushes all pending metadata writes. > static nsresult ShutdownMetadataWriteScheduling(); > > static nsresult OpenFile(const nsACString &aKey, > + uint32_t aFlags, bool aSyncResult, aSyncResult is an incorrect name in this case. It is OK in the events since there it is really a synchronous result, but these 2 methods always post an event to IO thread so this isn't and never can be a synchronous result. Rename it to something like aResultOnAnyThread, but feel free to find a better name :)
Attachment #8410971 - Flags: review?(michal.novotny) → review+
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
You need to log in before you can comment on or make changes to this bug.