Last Comment Bug 689008 - Optimize XHR2 blob response
: Optimize XHR2 blob response
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM: Core & HTML (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla10
Assigned To: Masatoshi Kimura [:emk]
:
Mentors:
Depends on: 697689
Blocks: 696586
  Show dependency treegraph
 
Reported: 2011-09-24 19:51 PDT by Masatoshi Kimura [:emk]
Modified: 2011-10-27 05:04 PDT (History)
6 users (show)
bzbarsky: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (3.49 KB, patch)
2011-09-24 19:51 PDT, Masatoshi Kimura [:emk]
no flags Details | Diff | Review
Part 1 - Skip reading already cached or local resource for blob response (3.62 KB, patch)
2011-09-27 14:55 PDT, Masatoshi Kimura [:emk]
no flags Details | Diff | Review
Part 2 - Fix the chache issue (1.47 KB, patch)
2011-09-27 14:56 PDT, Masatoshi Kimura [:emk]
no flags Details | Diff | Review
Part 3 - Test (6.37 KB, patch)
2011-09-27 14:56 PDT, Masatoshi Kimura [:emk]
jonas: review-
Details | Diff | Review
Part 2 - Fix the cache issue (1.56 KB, patch)
2011-09-28 16:08 PDT, Masatoshi Kimura [:emk]
no flags Details | Diff | Review
Part 1 - Skip reading already cached or local resource for blob response (3.60 KB, patch)
2011-09-29 14:21 PDT, Masatoshi Kimura [:emk]
jonas: review+
Details | Diff | Review
Part 2 - Fix the cache issue (1.56 KB, patch)
2011-09-29 14:22 PDT, Masatoshi Kimura [:emk]
michal.novotny: review+
Details | Diff | Review
Part 1 - Skip reading already cached or local resource for blob response, v3 (3.86 KB, patch)
2011-10-11 08:38 PDT, Masatoshi Kimura [:emk]
jonas: review+
Details | Diff | Review
Part 3 - Tests, v2 (5.89 KB, patch)
2011-10-11 08:39 PDT, Masatoshi Kimura [:emk]
jonas: review+
Details | Diff | Review
Part 2 - Fix the cache issue, for checkin (1.79 KB, patch)
2011-10-21 06:36 PDT, Masatoshi Kimura [:emk]
VYV03354: review+
Details | Diff | Review
Part 1 - Skip reading already cached or local resource for blob response, for checkin (3.86 KB, patch)
2011-10-23 03:43 PDT, Masatoshi Kimura [:emk]
VYV03354: review+
Details | Diff | Review
Part 1 - Skip reading already cached or local resource for blob response, for checkin (3.86 KB, patch)
2011-10-23 03:54 PDT, Masatoshi Kimura [:emk]
no flags Details | Diff | Review
Part 2 - Fix the cache issue, for checkin (1.80 KB, patch)
2011-10-23 03:55 PDT, Masatoshi Kimura [:emk]
no flags Details | Diff | Review
Part 3 - Tests (5.90 KB, patch)
2011-10-23 03:56 PDT, Masatoshi Kimura [:emk]
no flags Details | Diff | Review

Description Masatoshi Kimura [:emk] 2011-09-24 19:51:16 PDT
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.
Comment 1 Jonas Sicking (:sicking) 2011-09-26 14:06:27 PDT
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] 2011-09-26 15:42:00 PDT
(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) 2011-09-26 16:27:29 PDT
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 [:jduell] (needinfo? me) 2011-09-26 17:43:50 PDT
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 [:jduell] (needinfo? me) 2011-09-26 21:01:59 PDT
> // 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) 2011-09-26 23:29:28 PDT
Though if you were wanting to remove that, we can easily work around it.
Comment 7 Masatoshi Kimura [:emk] 2011-09-27 10:12:12 PDT
Comment on attachment 562273 [details] [diff] [review]
patch

Canceling review until the test problem is resolved.
Comment 8 Masatoshi Kimura [:emk] 2011-09-27 14:55:06 PDT
Created attachment 562875 [details] [diff] [review]
Part 1 - Skip reading already cached or local resource for blob response
Comment 9 Masatoshi Kimura [:emk] 2011-09-27 14:56:07 PDT
Created attachment 562876 [details] [diff] [review]
Part 2 - Fix the chache issue

I think I found a cause.
Comment 10 Masatoshi Kimura [:emk] 2011-09-27 14:56:46 PDT
Created attachment 562878 [details] [diff] [review]
Part 3 - Test
Comment 11 Masatoshi Kimura [:emk] 2011-09-27 17:36:29 PDT
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] 2011-09-28 16:08:18 PDT
Created attachment 563220 [details] [diff] [review]
Part 2 - Fix the cache issue

Marking valid only when the cache is stored on the disk file.
Comment 13 Masatoshi Kimura [:emk] 2011-09-28 19:04:25 PDT
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 [:jduell] (needinfo? me) 2011-09-29 13:24:41 PDT
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] 2011-09-29 14:21:34 PDT
Created attachment 563552 [details] [diff] [review]
Part 1 - Skip reading already cached or local resource for blob response

rebased to tip
Comment 16 Masatoshi Kimura [:emk] 2011-09-29 14:22:36 PDT
Created attachment 563553 [details] [diff] [review]
Part 2 - Fix the cache issue

rebased to tip
Comment 17 Jonas Sicking (:sicking) 2011-10-10 17:10:16 PDT
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) 2011-10-10 18:03:56 PDT
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) 2011-10-10 18:05:43 PDT
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] 2011-10-11 07:26:53 PDT
(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] 2011-10-11 08:38:08 PDT
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.
Comment 22 Masatoshi Kimura [:emk] 2011-10-11 08:39:26 PDT
Created attachment 566231 [details] [diff] [review]
Part 3 - Tests, v2

Resolved review comments.
Comment 23 Jonas Sicking (:sicking) 2011-10-13 13:55:15 PDT
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] 2011-10-19 18:32:18 PDT
Michal, ping?
Comment 25 Michal Novotny (:michal) 2011-10-20 13:31:04 PDT
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] 2011-10-20 15:56:45 PDT
(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) 2011-10-20 16:09:13 PDT
(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) 2011-10-20 16:11:16 PDT
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] 2011-10-21 06:36:48 PDT
Created attachment 568642 [details] [diff] [review]
Part 2 - Fix the cache issue, for checkin

Thank you. I updated a comment. Transferring r+.
Comment 30 Dão Gottwald [:dao] 2011-10-23 01:56:38 PDT
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] 2011-10-23 03:43:48 PDT
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.
Comment 32 Masatoshi Kimura [:emk] 2011-10-23 03:47:32 PDT
> 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] 2011-10-23 03:54:47 PDT
Created attachment 568935 [details] [diff] [review]
Part 1 - Skip reading already cached or local resource for blob response, for checkin
Comment 34 Masatoshi Kimura [:emk] 2011-10-23 03:55:28 PDT
Created attachment 568936 [details] [diff] [review]
Part 2 - Fix the cache issue, for checkin
Comment 35 Masatoshi Kimura [:emk] 2011-10-23 03:56:26 PDT
Created attachment 568937 [details] [diff] [review]
Part 3 - Tests

Ah, I got it. Added reviewer names.
Comment 36 Ed Morley [:emorley] 2011-10-24 07:27:48 PDT
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 2011-10-24 11:30:23 PDT
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

Note You need to log in before you can comment on or make changes to this bug.