Scenario: - open an entry - found an existing one - preload starts - opening an input stream - read all data - close the input stream - all chunks released - entry left in cache storage service hash tables with only the metadata - after a while, someone wants to read the entry again - opens an input stream (this for http channels happens on the cache I/O thread asap) - 1) - entry may be revalidated optionally - later asyncwait is called on the entry from ReadFromCache, on the main thread - 2) Preload currently starts at 2) but we can kick it at 1) i.e. much sooner.

Turns out to be that simple. Every time cache entry opens an input stream it also Seek(offset)'s on it. EnsureCorrectChunk calls on the file to get the appropriate chunk at the offset which also starts the preload. Michal, any conditions when we should only release the chunk?

Talos: https://tbpl.mozilla.org/?tree=Try&rev=85c54f4e09c4 (m-c with cache2) https://tbpl.mozilla.org/?tree=Try&rev=2e2ba95341ea (+this bug +bug 1013333)

Comment on attachment 8425817 [details] [diff] [review] v1 Review of attachment 8425817 [details] [diff] [review]: ----------------------------------------------------------------- (In reply to Honza Bambas (:mayhemer) from comment #1) > Michal, any conditions when we should only release the chunk? No, aReleaseOnly argument can be removed from input stream.

Talos doesn't show that much difference. I will test manually when there is time.

And here is the link: http://compare-talos.mattn.ca/?oldRevs=85c54f4e09c4&newRev=2e2ba95341ea&server=graphs.mozilla.org&submit=true

Manual tests show this has a meaning. Again testing with page2 of my blog only and a microsd. Here are the numbers: Cache version Warm go to cache v1 560ms / 440ms cache v2 710ms / 540ms with 1013587 615ms / 455ms [ complete page load time / first paint time ] So, we are winning here almost 100ms of first paint time!

Should be landed for 33.

One more try before landing: https://tbpl.mozilla.org/?tree=Try&rev=582e1f0422fa

Comment on attachment 8425817 [details] [diff] [review] v1 [Approval Request Comment] Bug caused by (feature/regressing bug #): none (or we could say turning the new HTTP cache on) User impact if declined: worse performance when loading a cached page for second time Testing completed (on m-c, etc.): 10 days on m-c, a lot of manual testing Risk to taking this patch (and alternatives if risky): nearly zero String or IDL/UUID changes made by this patch: none

Comment on attachment 8425817 [details] [diff] [review] v1 100ms improvement for a one line fix. Great win! Aurora approval granted.

Please back out ASAP, this has caused huge amounts of crashes and makes Aurora nearly unusable.

As a note, mayhemer said on IRC that the crashes were caused by this being uplifted without its prerequisite bug 1013638.

https://hg.mozilla.org/releases/mozilla-aurora/rev/8e6d0f924669 Backed out for crashes. See Bug 1031697. This bug/patch depends on Bug 1013638. Honza removed the flag a? in the other bug but missed removing it from this one causing us to land it into Aurora.

I reviewed this bug with jduell. Marking this small perf win as won't fix for 32.