Closed Bug 1013587 Opened 10 years ago Closed 10 years ago

[Perf] HTTP cache v2: Start preload on input stream open for existing entries

Categories

(Core :: Networking: Cache, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla33
Tracking Status
firefox32 + wontfix
firefox33 --- fixed
b2g-v2.0 --- affected
b2g-v2.1 --- fixed

People

(Reporter: mayhemer, Assigned: mayhemer)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

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.
Attached patch v1Splinter Review
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?
Assignee: nobody → honzab.moz
Status: NEW → ASSIGNED
Attachment #8425817 - Flags: review?(michal.novotny)
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.
Attachment #8425817 - Flags: review?(michal.novotny) → review+
Depends on: 1013638
Talos doesn't show that much difference. I will test manually when there is time.
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.
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
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
Attachment #8425817 - Flags: approval-mozilla-aurora?
Comment on attachment 8425817 [details] [diff] [review] v1 100ms improvement for a one line fix. Great win! Aurora approval granted.
Attachment #8425817 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Depends on: 1031697
Depends on: 1031852
Please back out ASAP, this has caused huge amounts of crashes and makes Aurora nearly unusable.
Attachment #8425817 - Flags: approval-mozilla-aurora+
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.
Depends on: 1039536
Depends on: 1042192
No longer depends on: 1042192
I reviewed this bug with jduell. Marking this small perf win as won't fix for 32.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: