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

Status: RESOLVED FIXED

(Core :: Networking: Cache, defect)

Description

[Honza Bambas (:mayhemer)]

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.

Comment 1

[Honza Bambas (:mayhemer)]

Attachment: v1

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?

Comment 2

[Honza Bambas (:mayhemer)]

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

[Michal Novotny [:michal]]

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.

Comment 4

[Honza Bambas (:mayhemer)]

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

Comment 5

[Honza Bambas (:mayhemer)]

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

Comment 6

[Honza Bambas (:mayhemer)]

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!

Comment 7

[Honza Bambas (:mayhemer)]

Should be landed for 33.

Comment 8

[Honza Bambas (:mayhemer)]

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

Comment 9

[Honza Bambas (:mayhemer)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/085ecf37395e

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/085ecf37395e

Comment 11

[Honza Bambas (:mayhemer)]

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 12

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8425817 [details] [diff] [review]
v1

100ms improvement for a one line fix. Great win! Aurora approval granted.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/80f77d10896b

Comment 14

[Jonathan Haas]

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

Comment 15

[Robert Kaiser]

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

Comment 16

[Nigel Babu [:nigelb]]

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.

Comment 17

[Lawrence Mandel [:lmandel] (use needinfo)]

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