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

Status: RESOLVED FIXED

(Core :: Networking: Cache, defect)

Description

[Honza Bambas (:mayhemer)]

Attachment: v1 - white space ignored for easier review

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

Comment 1

[Honza Bambas (:mayhemer)]

Talos try for the gearflower reg:  https://tbpl.mozilla.org/?tree=Try&rev=83b56c78d934

Comment 2

[Honza Bambas (:mayhemer)]

Apparently fixes the gearflower regression:

http://compare-talos.mattn.ca/?oldRevs=8ea0aaea5999&newRev=83b56c78d934&server=graphs.mozilla.org&submit=true

Comment 3

[Honza Bambas (:mayhemer)]

Attachment: v1 (backup)

Comment 4

[Honza Bambas (:mayhemer)]

https://tbpl.mozilla.org/?tree=Try&rev=66f32b3ba38c

Comment 5

[Honza Bambas (:mayhemer)]

According Read(), it should be based on ReadSegments and not reimplement all again itself.

Comment 6

[Michal Novotny [:michal]]

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.

Comment 7

[Honza Bambas (:mayhemer)]

(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.

Comment 8

[Honza Bambas (:mayhemer)]

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?

Comment 9

[Honza Bambas (:mayhemer)]

(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.

Comment 10

[Honza Bambas (:mayhemer)]

Attachment: v1.1 white space ignored

- 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)

Comment 11

[Honza Bambas (:mayhemer)]

Attachment: v1.1 [full patch]

Comment 12

[Michal Novotny [:michal]]

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.

Comment 13

[Honza Bambas (:mayhemer)]

Attachment: v1.2 [as landed]

Thanks!  Version to land with the nit fixed.

https://hg.mozilla.org/integration/mozilla-inbound/rev/826c86c100d8

Comment 14

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

https://hg.mozilla.org/mozilla-central/rev/826c86c100d8