Closed Bug 689008 Opened 8 years ago Closed 8 years ago

Optimize XHR2 blob response

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla10

People

(Reporter: emk, Assigned: emk)

References

Details

Attachments

(3 files, 11 obsolete files)

3.86 KB, patch
Details | Diff | Splinter Review
1.80 KB, patch
Details | Diff | Splinter Review
5.90 KB, patch
Details | Diff | Splinter Review
Attached patch patch (obsolete) — Splinter Review
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.
Attachment #562273 - Flags: review?(jonas)
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".
(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?
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?
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.
> // 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.
Though if you were wanting to remove that, we can easily work around it.
Comment on attachment 562273 [details] [diff] [review]
patch

Canceling review until the test problem is resolved.
Attachment #562273 - Flags: review?(jonas)
Assignee: nobody → VYV03354
Attachment #562273 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #562875 - Flags: review?(jonas)
Attached patch Part 2 - Fix the chache issue (obsolete) — Splinter Review
I think I found a cause.
Attachment #562876 - Flags: review?(jduell.mcbugs)
Attached patch Part 3 - Test (obsolete) — Splinter Review
Attachment #562878 - Flags: review?(jonas)
Comment on attachment 562876 [details] [diff] [review]
Part 2 - Fix the chache issue

Grr, it caused many necko test failures.
Attachment #562876 - Flags: review?(jduell.mcbugs)
Attached patch Part 2 - Fix the cache issue (obsolete) — Splinter Review
Marking valid only when the cache is stored on the disk file.
Attachment #562876 - Attachment is obsolete: true
Comment on attachment 563220 [details] [diff] [review]
Part 2 - Fix the cache issue

It looks to be passed the try, so requesting review.
Attachment #563220 - Flags: review?(jduell.mcbugs)
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.
Attachment #563220 - Flags: review?(jduell.mcbugs) → review?(michal.novotny)
rebased to tip
Attachment #562875 - Attachment is obsolete: true
Attachment #562875 - Flags: review?(jonas)
Attachment #563552 - Flags: review?(jonas)
Attached patch Part 2 - Fix the cache issue (obsolete) — Splinter Review
rebased to tip
Attachment #563220 - Attachment is obsolete: true
Attachment #563220 - Flags: review?(michal.novotny)
Attachment #563553 - Flags: review?(michal.novotny)
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 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.
Attachment #562878 - Flags: review?(jonas) → review-
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.
Attachment #563552 - Flags: review?(jonas) → review+
(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.
Added a comment to clarify the intention.
Attachment #563552 - Attachment is obsolete: true
Attachment #566230 - Flags: review?(jonas)
Attached patch Part 3 - Tests, v2 (obsolete) — Splinter Review
Resolved review comments.
Attachment #562878 - Attachment is obsolete: true
Attachment #566231 - Flags: review?(jonas)
Comment on attachment 566231 [details] [diff] [review]
Part 3 - Tests, v2

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

r=me
Attachment #566231 - Flags: review?(jonas) → review+
Michal, ping?
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...
(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?
(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 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.
Attachment #563553 - Flags: review?(michal.novotny) → review+
Thank you. I updated a comment. Transferring r+.
Attachment #563553 - Attachment is obsolete: true
Attachment #568642 - Flags: review+
Keywords: checkin-needed
Blocks: 696586
Part 1 fails to apply. I didn't try the others.
By the way, your commit messages lack the review part.
Keywords: checkin-needed
Ah, I've forgotten to attach an unbitrotted version for PRBool->bool migration.
Attachment #566230 - Attachment is obsolete: true
Attachment #568934 - Flags: review+
> 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
Keywords: checkin-needed
Attachment #568642 - Attachment is obsolete: true
Attached patch Part 3 - TestsSplinter Review
Ah, I got it. Added reviewer names.
Attachment #566231 - Attachment is obsolete: true
Has this been sent to try? If not, I'll do so before pushing, once the try situation is resolved (bug 696682).
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/VYV03354@nifty.ne.jp-05ad4052ca88
Depends on: 697689
You need to log in before you can comment on or make changes to this bug.