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

RESOLVED FIXED in Firefox 33

Status

()

defect
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: mayhemer, Assigned: mayhemer)

Tracking

(Blocks 1 bug)

Trunk
mozilla33
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox32+ wontfix, firefox33 fixed, b2g-v2.0 affected, b2g-v2.1 fixed)

Details

Attachments

(1 attachment)

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.
Posted 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.
https://hg.mozilla.org/mozilla-central/rev/085ecf37395e
Status: ASSIGNED → RESOLVED
Closed: 5 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.
Duplicate of this bug: 746018
You need to log in before you can comment on or make changes to this bug.