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

RESOLVED FIXED in Firefox 33

Status

()

RESOLVED FIXED
5 years ago
3 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)

(Assignee)

Description

5 years ago
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.
(Assignee)

Comment 1

5 years ago
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+
(Assignee)

Updated

5 years ago
Depends on: 1013638
(Assignee)

Comment 4

5 years ago
Talos doesn't show that much difference.  I will test manually when there is time.
(Assignee)

Comment 6

5 years ago
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!
(Assignee)

Comment 7

5 years ago
Should be landed for 33.
(Assignee)

Comment 8

5 years ago
One more try before landing: https://tbpl.mozilla.org/?tree=Try&rev=582e1f0422fa
https://hg.mozilla.org/mozilla-central/rev/085ecf37395e
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
(Assignee)

Comment 11

5 years ago
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?
status-firefox32: --- → affected
status-firefox33: --- → fixed
tracking-firefox32: --- → +
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+
https://hg.mozilla.org/releases/mozilla-aurora/rev/80f77d10896b
status-b2g-v2.0: --- → fixed
status-b2g-v2.1: --- → fixed
status-firefox32: affected → fixed

Updated

5 years ago
Depends on: 1031697

Updated

5 years ago
Depends on: 1031852

Comment 14

5 years ago
Please back out ASAP, this has caused huge amounts of crashes and makes Aurora nearly unusable.
(Assignee)

Updated

5 years ago
Attachment #8425817 - Flags: approval-mozilla-aurora+

Comment 15

5 years ago
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.
status-firefox32: fixed → affected
status-b2g-v2.0: fixed → affected
status-b2g-v2.1: fixed → affected
status-b2g-v2.1: affected → fixed
(Assignee)

Updated

5 years ago
Depends on: 1039536

Updated

5 years ago
Depends on: 1042192

Updated

5 years ago
No longer depends on: 1042192
I reviewed this bug with jduell. Marking this small perf win as won't fix for 32.
status-firefox32: affected → wontfix
(Assignee)

Updated

3 years ago
Duplicate of this bug: 746018
You need to log in before you can comment on or make changes to this bug.