Closed
Bug 999383
Opened 11 years ago
Closed 11 years ago
HTTP cache v2: optimize Open and Read inter-thread calls
Categories
(Core :: Networking: Cache, defect)
Tracking
()
RESOLVED
FIXED
mozilla31
People
(Reporter: mayhemer, Assigned: mayhemer)
References
Details
Attachments
(1 file)
(deleted),
patch
|
michal
:
review+
|
Details | Diff | Splinter Review |
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)
Assignee | ||
Comment 1•11 years ago
|
||
Michal, should I make this optional or is it OK to always to call the result directly all the time?
Flags: needinfo?(michal.novotny)
Assignee | ||
Comment 2•11 years ago
|
||
Assignee | ||
Comment 3•11 years ago
|
||
complete run:
https://tbpl.mozilla.org/?tree=Try&rev=90b1635eb995
Assignee | ||
Comment 4•11 years ago
|
||
Doesn't help with bug 993649
Comment 5•11 years ago
|
||
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.
Flags: needinfo?(michal.novotny)
Assignee | ||
Comment 6•11 years ago
|
||
- 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)
Assignee | ||
Comment 7•11 years ago
|
||
Comment 8•11 years ago
|
||
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+
Assignee | ||
Comment 9•11 years ago
|
||
Comment 10•11 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
You need to log in
before you can comment on or make changes to this bug.
Description
•