689008 - Optimize XHR2 blob response

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Masatoshi Kimura [:emk]]

Attachment: patch

It's a waste of time to read through from already cached or local content.
This patch will make blob response much faster regardless of the response size.

Comment 1

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I'd really like to see tests for this. For example, will this fire a "progress" event with the proper values for .loaded/.total/.lengthComputable? And will "load" and "loadend" get the proper values for those properties?

You can probably add to the test_xhr_progressevents.html test to add a load of a large cached resource when .responseType is "blob".

Comment 2

[Masatoshi Kimura [:emk]]

(In reply to Jonas Sicking (:sicking) from comment #1)
If I send the second consecutive sync request to the same resource, sometimes the request fails to open the cache because the cache is "busy". If I send the async request, callback will not be fired until the first response is GCed (it took 15 minutes on my test). If I hold the reference to the first response, the callback will never be fired. This bug (?) seems to be unrelated to the patch.
Is there a reliable way to ensure the resource is cached without disturbing the second request? Or should I investigate this bug?

Comment 3

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

CC'ing Jason for help on the cache issue.

You say "If I hold the reference to the first response, the callback will never be fired". What callback are you referring to? And is the fact that it doesn't fire the bug you are referring to in the following sentence?

Comment 4

[Jason Duell]

This could be the issue we have where when an initial channel creates and opens a cache entry for writing, the 2nd and following requests to that URI will block until the writer releases the cache entry.  In a regular, successful request that's done in OnStopRequest.  I'm not clear on why it's not happening if you hang onto the XHR request itself.  But if the underlying nsHTTPChannel's mCacheEntry is not null for the first request, it'll block other channels from reading the cache entry indefinitely.

Comment 5

[Jason Duell]

> // We don't have to read from the local file for the blob response
> return request->Cancel(NS_OK);

Aha--I finally encounter a case where channel->Cancel(NS_OK) makes sense.  I've been wondering why we support that.

Comment 6

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Though if you were wanting to remove that, we can easily work around it.

Comment 7

[Masatoshi Kimura [:emk]]

Comment on attachment 562273 [details] [diff] [review]
patch

Canceling review until the test problem is resolved.

Comment 8

[Masatoshi Kimura [:emk]]

Attachment: Part 1 - Skip reading already cached or local resource for blob response

Comment 9

[Masatoshi Kimura [:emk]]

Attachment: Part 2 - Fix the chache issue

I think I found a cause.

Comment 10

[Masatoshi Kimura [:emk]]

Attachment: Part 3 - Test

Comment 11

[Masatoshi Kimura [:emk]]

Comment on attachment 562876 [details] [diff] [review]
Part 2 - Fix the chache issue

Grr, it caused many necko test failures.

Comment 12

[Masatoshi Kimura [:emk]]

Attachment: Part 2 - Fix the cache issue

Marking valid only when the cache is stored on the disk file.

Comment 13

[Masatoshi Kimura [:emk]]

Comment on attachment 563220 [details] [diff] [review]
Part 2 - Fix the cache issue

It looks to be passed the try, so requesting review.

Comment 14

[Jason Duell]

Comment on attachment 563220 [details] [diff] [review]
Part 2 - Fix the cache issue

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

Michal,  can you take this (or pass to bjarne/nick or someone else working on the cache)?  Needed for newest version of XMLHttpRequest to work.

Comment 15

[Masatoshi Kimura [:emk]]

Attachment: Part 1 - Skip reading already cached or local resource for blob response

rebased to tip

Comment 16

[Masatoshi Kimura [:emk]]

Attachment: Part 2 - Fix the cache issue

rebased to tip

Comment 17

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 563552 [details] [diff] [review]
Part 1 - Skip reading already cached or local resource for blob response

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

::: content/base/src/nsXMLHttpRequest.cpp
@@ +1701,5 @@
>      if (fc) {
>        fc->GetFile(getter_AddRefs(file));
>      }
>    }
> +  bool fromFile = false;

Seems cleaner to move the logic for setting fromFile to the if-statement above. Can GetCacheFile really return a file while IsFromCache returns false? I.e. can fromFile ever be false when file is true?

Comment 18

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 562878 [details] [diff] [review]
Part 3 - Test

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

::: content/base/test/test_xhr_progressevents.html
@@ +152,5 @@
>                   { data: utf8encode("a\u867Eb").substr(3,1), utf16: "\u867E" },
>                   { data: utf8encode("a\u867Eb").substr(4), utf16: "b" },
>                   { close: true },
> +                 { file: "file_XHR_binary2.bin", name: "cacheable data", total: 65536 },
> +                 { file: "file_XHR_binary2.bin", name: "cached data", total: 65536 },

Split these into two entries each, like:

{ file: ... },
{ close: true },

That way you don't have to duplicate the loop which waits for data to come in.

@@ +160,5 @@
>      for (let i = 0; i < tests.length; ++i) {
>        let test = tests[i];
> +      if (responseType.type !== "blob" && "file" in test) {
> +        continue;
> +      }

Instead of doing it this way, add something like:

if (responseType.type === "blob") {
  tests.push({ file: ... },
             { file: ... });
}

just after the tests array is created, outside the inner loop.

@@ +175,5 @@
>                        encoded: test.encoded,
>                        nodata: responseType.nodata,
>                        chunked: responseType.chunked,
> +                      text: responseType.text,
> +                      cacheable: "file" in test };

I'd just do:

file: test.file

That way the file-specific behavior is more specific. We might want to add tests for cachable non-file resources later.

@@ +180,4 @@
>    
>          xhr.onreadystatechange = null;
> +        if (testState.cacheable)
> +          xhr.open("GET", test.file);

Then you can use testState.file in both these locations.

@@ +199,5 @@
>        }
> +      if ("file" in test) {
> +        testState.pendingBytes = testState.total;
> +        testState.pendingResult = fileExpectedResult;
> +        while(testState.pendingBytes) {

By breaking out the 'close' entry as described above, you can remove this while-loop.

Comment 19

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 563552 [details] [diff] [review]
Part 1 - Skip reading already cached or local resource for blob response

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

r=me with that fixed.

Comment 20

[Masatoshi Kimura [:emk]]

(In reply to Jonas Sicking (:sicking) from comment #17)
> Seems cleaner to move the logic for setting fromFile to the if-statement
> above. Can GetCacheFile really return a file while IsFromCache returns
> false? I.e. can fromFile ever be false when file is true?
GetCacheFile will return the partial or empty file when the response is going be cached. In this case, file will be non-null and fromFile will be false.
We should not skip reading from the network unless the response is fully cached. Otherwise, we will return the broken blob response to the content.

Comment 21

[Masatoshi Kimura [:emk]]

Attachment: Part 1 - Skip reading already cached or local resource for blob response, v3

Added a comment to clarify the intention.

Comment 22

[Masatoshi Kimura [:emk]]

Attachment: Part 3 - Tests, v2

Resolved review comments.

Comment 23

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 566231 [details] [diff] [review]
Part 3 - Tests, v2

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

r=me

Comment 24

[Masatoshi Kimura [:emk]]

Michal, ping?

Comment 25

[Michal Novotny [:michal]]

Comment on attachment 563553 [details] [diff] [review]
Part 2 - Fix the cache issue

> +   NS_SUCCEEDED(GetCacheAsFile(&asFile)) && asFile) {

Why is this limited to entries that are stored as file?

Otherwise looks good. And sorry for the delay, the request for review got lost in my mailbox...

Comment 26

[Masatoshi Kimura [:emk]]

(In reply to Michal Novotny (:michal) from comment #25)
> Comment on attachment 563553 [details] [diff] [review] [diff] [details] [review]
> Part 2 - Fix the cache issue
> 
> > +   NS_SUCCEEDED(GetCacheAsFile(&asFile)) && asFile) {
> 
> Why is this limited to entries that are stored as file?
> 
> Otherwise looks good. And sorry for the delay, the request for review got
> lost in my mailbox...

This patch causes test failures if this check is removed (see comment #11). Do I have to investigate further?

Comment 27

[Michal Novotny [:michal]]

(In reply to Masatoshi Kimura [:emk] from comment #26)
> This patch causes test failures if this check is removed (see comment #11).
> Do I have to investigate further?

Ah, I think I know what causes the failures. We write to the cache asynchronously when it isn't stored in the file and it isn't completely written to the disk yet in nsHttpChannel::OnStopRequest().

Comment 28

[Michal Novotny [:michal]]

Comment on attachment 563553 [details] [diff] [review]
Part 2 - Fix the cache issue

Maybe add the information from comment #27 to the comment in the source file.

Comment 29

[Masatoshi Kimura [:emk]]

Attachment: Part 2 - Fix the cache issue, for checkin

Thank you. I updated a comment. Transferring r+.

Comment 30

[Dão Gottwald [:dao]]

Part 1 fails to apply. I didn't try the others.
By the way, your commit messages lack the review part.

Comment 31

[Masatoshi Kimura [:emk]]

Attachment: Part 1 - Skip reading already cached or local resource for blob response, for checkin

Ah, I've forgotten to attach an unbitrotted version for PRBool->bool migration.

Comment 32

[Masatoshi Kimura [:emk]]

> By the way, your commit messages lack the review part.
Sorry, I don't understand what does it mean.
Here are commit messages from the patch files.
> Bug 689008: Part 1 - Skip reading already cached or local resource for blob response
> Bug 689008: Part 2 - Fix the cache issue
> Bug 689008: Part 3 - Test

Comment 33

[Masatoshi Kimura [:emk]]

Attachment: Part 1 - Skip reading already cached or local resource for blob response, for checkin

Comment 34

[Masatoshi Kimura [:emk]]

Attachment: Part 2 - Fix the cache issue, for checkin

Comment 35

[Masatoshi Kimura [:emk]]

Attachment: Part 3 - Tests

Ah, I got it. Added reviewer names.

Comment 36

[Ed Morley [:emorley]]

Has this been sent to try? If not, I'll do so before pushing, once the try situation is resolved (bug 696682).

Comment 37

[Mozilla RelEng Bot]

Try run for 05ad4052ca88 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=05ad4052ca88
Results (out of 190 total builds):
    success: 183
    warnings: 7
Builds available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-05ad4052ca88

Comment 38

[Boris Zbarsky [:bzbarsky]]

Pushed:
  http://hg.mozilla.org/integration/mozilla-inbound/rev/5db5141fb595
  http://hg.mozilla.org/integration/mozilla-inbound/rev/e050ed22381c
  http://hg.mozilla.org/integration/mozilla-inbound/rev/254bfb46c100

Comment 39

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/5db5141fb595
https://hg.mozilla.org/mozilla-central/rev/e050ed22381c
https://hg.mozilla.org/mozilla-central/rev/254bfb46c100