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)
Core
Networking: Cache
Tracking
()
RESOLVED
FIXED
mozilla33
People
(Reporter: mayhemer, Assigned: mayhemer)
References
(Blocks 1 open bug)
Details
Attachments
(1 file)
723 bytes,
patch
|
michal
:
review+
|
Details | Diff | Splinter Review |
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•10 years ago
|
||
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)
Assignee | ||
Comment 2•10 years ago
|
||
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 3•10 years ago
|
||
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 | ||
Comment 4•10 years ago
|
||
Talos doesn't show that much difference. I will test manually when there is time.
Assignee | ||
Comment 5•10 years ago
|
||
Assignee | ||
Comment 6•10 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•10 years ago
|
||
Should be landed for 33.
Assignee | ||
Comment 8•10 years ago
|
||
One more try before landing: https://tbpl.mozilla.org/?tree=Try&rev=582e1f0422fa
Assignee | ||
Comment 9•10 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
Assignee | ||
Comment 11•10 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?
Updated•10 years ago
|
Comment 12•10 years ago
|
||
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+
Comment 13•10 years ago
|
||
Comment 14•10 years ago
|
||
Please back out ASAP, this has caused huge amounts of crashes and makes Aurora nearly unusable.
Assignee | ||
Updated•10 years ago
|
Attachment #8425817 -
Flags: approval-mozilla-aurora+
Comment 15•10 years ago
|
||
As a note, mayhemer said on IRC that the crashes were caused by this being uplifted without its prerequisite bug 1013638.
Comment 16•10 years ago
|
||
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.
Updated•10 years ago
|
Updated•10 years ago
|
Updated•10 years ago
|
Comment 17•10 years ago
|
||
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.
Description
•