Optimize XHR2 blob response

RESOLVED FIXED in mozilla10

Status

()

Core
DOM: Core & HTML
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: emk, Assigned: emk)

Tracking

unspecified
mozilla10
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 11 obsolete attachments)

3.86 KB, patch
Details | Diff | Splinter Review
1.80 KB, patch
Details | Diff | Splinter Review
5.90 KB, patch
Details | Diff | Splinter Review
(Assignee)

Description

6 years ago
Created attachment 562273 [details] [diff] [review]
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.
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".
(Assignee)

Comment 2

6 years ago
(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.
(Assignee)

Comment 7

6 years ago
Comment on attachment 562273 [details] [diff] [review]
patch

Canceling review until the test problem is resolved.
Attachment #562273 - Flags: review?(jonas)
(Assignee)

Comment 8

6 years ago
Created attachment 562875 [details] [diff] [review]
Part 1 - Skip reading already cached or local resource for blob response
Assignee: nobody → VYV03354
Attachment #562273 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #562875 - Flags: review?(jonas)
(Assignee)

Comment 9

6 years ago
Created attachment 562876 [details] [diff] [review]
Part 2 - Fix the chache issue

I think I found a cause.
Attachment #562876 - Flags: review?(jduell.mcbugs)
(Assignee)

Comment 10

6 years ago
Created attachment 562878 [details] [diff] [review]
Part 3 - Test
Attachment #562878 - Flags: review?(jonas)
(Assignee)

Comment 11

6 years ago
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)
(Assignee)

Comment 12

6 years ago
Created attachment 563220 [details] [diff] [review]
Part 2 - Fix the cache issue

Marking valid only when the cache is stored on the disk file.
Attachment #562876 - Attachment is obsolete: true
(Assignee)

Comment 13

6 years ago
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)
(Assignee)

Comment 15

6 years ago
Created attachment 563552 [details] [diff] [review]
Part 1 - Skip reading already cached or local resource for blob response

rebased to tip
Attachment #562875 - Attachment is obsolete: true
Attachment #562875 - Flags: review?(jonas)
Attachment #563552 - Flags: review?(jonas)
(Assignee)

Comment 16

6 years ago
Created attachment 563553 [details] [diff] [review]
Part 2 - Fix the cache issue

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+
(Assignee)

Comment 20

6 years ago
(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.
(Assignee)

Comment 21

6 years ago
Created attachment 566230 [details] [diff] [review]
Part 1 - Skip reading already cached or local resource for blob response, v3

Added a comment to clarify the intention.
Attachment #563552 - Attachment is obsolete: true
Attachment #566230 - Flags: review?(jonas)
(Assignee)

Comment 22

6 years ago
Created attachment 566231 [details] [diff] [review]
Part 3 - Tests, v2

Resolved review comments.
Attachment #562878 - Attachment is obsolete: true
Attachment #566231 - Flags: review?(jonas)
Attachment #566230 - Flags: review?(jonas) → review+
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+
(Assignee)

Comment 24

6 years ago
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...
(Assignee)

Comment 26

6 years ago
(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+
(Assignee)

Comment 29

6 years ago
Created attachment 568642 [details] [diff] [review]
Part 2 - Fix the cache issue, for checkin

Thank you. I updated a comment. Transferring r+.
Attachment #563553 - Attachment is obsolete: true
Attachment #568642 - Flags: review+
(Assignee)

Updated

6 years ago
Keywords: checkin-needed
(Assignee)

Updated

6 years ago
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
(Assignee)

Comment 31

6 years ago
Created attachment 568934 [details] [diff] [review]
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.
Attachment #566230 - Attachment is obsolete: true
Attachment #568934 - Flags: review+
(Assignee)

Comment 32

6 years ago
> 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
(Assignee)

Comment 33

6 years ago
Created attachment 568935 [details] [diff] [review]
Part 1 - Skip reading already cached or local resource for blob response, for checkin
Attachment #568934 - Attachment is obsolete: true
(Assignee)

Comment 34

6 years ago
Created attachment 568936 [details] [diff] [review]
Part 2 - Fix the cache issue, for checkin
Attachment #568642 - Attachment is obsolete: true
(Assignee)

Comment 35

6 years ago
Created attachment 568937 [details] [diff] [review]
Part 3 - Tests

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

Comment 37

6 years ago
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
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
Flags: in-testsuite+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/5db5141fb595
https://hg.mozilla.org/mozilla-central/rev/e050ed22381c
https://hg.mozilla.org/mozilla-central/rev/254bfb46c100
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla10
(Assignee)

Updated

6 years ago
Depends on: 697689
You need to log in before you can comment on or make changes to this bug.