Closed Bug 1010221 Opened 6 years ago Closed 6 years ago

HTTP cache v2: Let CacheFileInputStream::ReadSegments loop over all preloaded chunks

Categories

(Core :: Networking: Cache, defect, major)

defect
Not set
major

Tracking

()

RESOLVED FIXED
mozilla32

People

(Reporter: mayhemer, Assigned: mayhemer)

References

Details

Attachments

(1 file, 4 obsolete files)

According [1] we can call the writer function multiple times.  Since bug 913819 we preload chunks in advance.  But we still let the input stream pump redispatch ODA over each of them.  On slower hardware or flooded main thread loop this will slow loading larger payload down significantly - I think it's cause of the And 4.0 tsvgx/gearflower regression.

What the patch does:
- new CacheFile::BytesFromChunk returns the number of available bytes from the given chunk index, the result is used for CacheFileInputStream::Available() result
- CacheFileInputStream::ReadSegments may then get aCount > chunk_size ; if we can get a next chunk from CacheFile immediately (via call to EnsureCorrectChunk) we loop the process and call on aWriter again with next chunk data immediately from within ReadSegments 


[1] http://mxr.mozilla.org/mozilla-central/source/xpcom/io/nsIInputStream.idl#110
Attachment #8422392 - Flags: feedback?(michal.novotny)
Talos try for the gearflower reg:  https://tbpl.mozilla.org/?tree=Try&rev=83b56c78d934
Attached patch v1 (backup) (obsolete) — Splinter Review
Attachment #8422392 - Attachment description: wip1 - white space ignored for easer review → v1 - white space ignored for easier review
According Read(), it should be based on ReadSegments and not reimplement all again itself.
Blocks: cache2enable
No longer blocks: 982598
Comment on attachment 8422392 [details] [diff] [review]
v1 - white space ignored for easier review

Review of attachment 8422392 [details] [diff] [review]:
-----------------------------------------------------------------

The same change should be done also in Read() method.

::: netwerk/cache2/CacheFile.cpp
@@ +1379,5 @@
> +    CacheFileChunk * chunk;
> +
> +    chunk = mChunks.GetWeak(i);
> +    if (chunk && chunk->IsReady())
> +      continue;

Maybe just add a following assertion:

MOZ_ASSERT(i == lastChunk || chunk->DataSize() == kChunkSize);

@@ +1381,5 @@
> +    chunk = mChunks.GetWeak(i);
> +    if (chunk && chunk->IsReady())
> +      continue;
> +
> +    chunk = mCachedChunks.GetWeak(i);

We should not search the chunk in mCachedChunks when it is in mChunks but it isn't ready.

::: netwerk/cache2/CacheFileInputStream.cpp
@@ +88,5 @@
>  
>    *_retval = 0;
>  
>    if (mChunk) {
> +    int64_t canRead = mFile->BytesFromChunk(mChunk->Index());

Available() can return a larger value than the following ReadSegments() can really read (in case CacheFile::ThrowMemoryCachedData() is called between those 2 calls). Is this OK?

@@ +204,3 @@
>    EnsureCorrectChunk(false);
> +    }
> +

I think that while loop should start here. EnsureCorrectChunk() is called only once at the start and we don't have to check whether the stream is closed every time we process a new chunk. If any error occurs when we read the data in the loop, the if-statement below will handle it.

@@ +207,5 @@
>    if (NS_FAILED(mStatus))
>      return mStatus;
>  
>    if (!mChunk) {
> +      if (mListeningForChunk == -1 || *_retval) {

We cannot be here if *_retval is non-zero, since then it is guaranteed that we have mChunk.
Attachment #8422392 - Flags: feedback?(michal.novotny) → feedback+
(In reply to Michal Novotny (:michal) from comment #6)
> Comment on attachment 8422392 [details] [diff] [review]
> v1 - white space ignored for easier review
> 
> Review of attachment 8422392 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The same change should be done also in Read() method.

Filed bug 1010783 for it, no need to bother with it now.

> Available() can return a larger value than the following ReadSegments() can
> really read (in case CacheFile::ThrowMemoryCachedData() is called between
> those 2 calls). Is this OK?

Yes, that is OK.  Read/Segments are not obligated to return what Available announces.  Also, ThrowMemoryCachedData() is currently unused.  Though, e.g. the image lib code wants count == read when reading the data.  Also I think it's a precondition for input stream pump to make it work (consume everything from the stream), but I've heard about a will to loose this requirement.  If there are problems, we can handle them later, try is quiet.
BTW, what about the |EnsureCorrectChunk(false); // TODO fix when only to release...| ?  I don't much follow the condition but if I understand correctly, you want to release the chunk if we reached the end of the stream?
(In reply to Honza Bambas (:mayhemer) from comment #8)
> BTW, what about the |EnsureCorrectChunk(false); // TODO fix when only to
> release...| ?  I don't much follow the condition but if I understand
> correctly, you want to release the chunk if we reached the end of the stream?

Discussed on IRC, passing |false| is correct.
Attached patch v1.1 white space ignored (obsolete) — Splinter Review
- all review comments addressed where applicable
- condition for EnsureCorrectChunk at the end of reading left at false

https://tbpl.mozilla.org/?tree=Try&rev=13bd1cfa376e (try -a)
https://tbpl.mozilla.org/?tree=Try&rev=2e97a067c7f7 (android talos)
Attachment #8423109 - Flags: review?(michal.novotny)
Attached patch v1.1 [full patch] (obsolete) — Splinter Review
Attachment #8422392 - Attachment is obsolete: true
Attachment #8422455 - Attachment is obsolete: true
Comment on attachment 8423109 [details] [diff] [review]
v1.1 white space ignored

Review of attachment 8423109 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with the tiny change

::: netwerk/cache2/CacheFileInputStream.cpp
@@ +204,1 @@
>    EnsureCorrectChunk(false);

The while loop should start after this call to EnsureCorrectChunk(). Sorry if it wasn't clear from my comment.
Attachment #8423109 - Flags: review?(michal.novotny) → review+
Attached patch v1.2 [as landed]Splinter Review
Thanks!  Version to land with the nit fixed.

https://hg.mozilla.org/integration/mozilla-inbound/rev/826c86c100d8
Attachment #8423109 - Attachment is obsolete: true
Attachment #8423110 - Attachment is obsolete: true
Attachment #8423309 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/826c86c100d8
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
Depends on: 1011873
No longer depends on: 1011873
You need to log in before you can comment on or make changes to this bug.