HTTP cache v2: optimize Open and Read inter-thread calls

RESOLVED FIXED in mozilla31

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: mayhemer, Assigned: mayhemer)

Tracking

Other Branch
mozilla31
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

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?
Flags: needinfo?(michal.novotny)
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.
Flags: needinfo?(michal.novotny)
Posted patch v1Splinter Review
- 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+
https://hg.mozilla.org/mozilla-central/rev/9f685a4d608f
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
Duplicate of this bug: 988011
You need to log in before you can comment on or make changes to this bug.