Last Comment Bug 744719 - Don't download appcache files one file at a time
: Don't download appcache files one file at a time
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Networking: Cache (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla15
Assigned To: Honza Bambas (:mayhemer)
:
Mentors:
Depends on:
Blocks: 702369 756620
  Show dependency treegraph
 
Reported: 2012-04-12 02:02 PDT by Jonas Sicking (:sicking) No longer reading bugmail consistently
Modified: 2012-06-04 07:14 PDT (History)
9 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+


Attachments
v1 (17.09 KB, patch)
2012-05-04 11:19 PDT, Honza Bambas (:mayhemer)
no flags Details | Diff | Splinter Review
v1.1 (17.04 KB, patch)
2012-05-14 11:10 PDT, Honza Bambas (:mayhemer)
michal.novotny: review-
Details | Diff | Splinter Review
v2 (28.05 KB, patch)
2012-05-26 06:52 PDT, Honza Bambas (:mayhemer)
michal.novotny: review+
philringnalda: checkin-
Details | Diff | Splinter Review
v3 (34.89 KB, patch)
2012-05-30 04:41 PDT, Honza Bambas (:mayhemer)
michal.novotny: review+
honzab.moz: checkin+
Details | Diff | Splinter Review
interdiff v2->v3 (11.28 KB, patch)
2012-05-30 04:42 PDT, Honza Bambas (:mayhemer)
no flags Details | Diff | Splinter Review

Description Jonas Sicking (:sicking) No longer reading bugmail consistently 2012-04-12 02:02:57 PDT
Apparently we currently download appcache resources one file at a time. This results in slower downloads. Ideal would be if the nsIOfflineCacheUpdate interface allowed setting "download at the most X files at a time" or some such. That way we can when pinning an app aggressively download the app, while for normal updates download a bit more slowly to avoid tying up the network.

I'm not sure if "download at the most X files at a time" is the right way to tweak bandwidth use. Definitely open to other ideas from people that know network stuff better than me.
Comment 1 Honza Bambas (:mayhemer) 2012-04-12 07:04:24 PDT
I believe we may just run them all in parallel and the http scheduling layer will take care automatically.

I'd like to work on this.
Comment 2 Johnny Stenback (:jst, jst@mozilla.com) 2012-05-02 17:38:00 PDT
I personally feel that requiring authors to maintain an accurate size in the manifest file is bound to be troublesome. I'd much prefer a small update time penalty to figure out the size ourselves over assuming that people will consistently get the size in the manifest updated correctly.
Comment 3 Honza Bambas (:mayhemer) 2012-05-02 17:43:37 PDT
(In reply to Johnny Stenback (:jst, jst@mozilla.com) from comment #2)
> I personally feel that requiring authors to maintain an accurate size in the
> manifest file is bound to be troublesome. I'd much prefer a small update
> time penalty to figure out the size ourselves over assuming that people will
> consistently get the size in the manifest updated correctly.

Agree.  Doesn't this comment belong to bug 744713 ?

On a subsequent update we can potentially estimate the new cache version size from the current cache version we have.
Comment 4 Johnny Stenback (:jst, jst@mozilla.com) 2012-05-02 18:38:52 PDT
Duh, yes, typed that in the wrong tab here. :( Ignore comment 2.
Comment 5 Johnny Stenback (:jst, jst@mozilla.com) 2012-05-03 15:58:50 PDT
This blocks our webapps story, so blocking kilimanjaro.
Comment 6 Honza Bambas (:mayhemer) 2012-05-04 11:19:49 PDT
Created attachment 621111 [details] [diff] [review]
v1

Needs more testing (I'll create test just for this new code), but so far seems to be working well.
Comment 7 Honza Bambas (:mayhemer) 2012-05-14 11:10:44 PDT
Created attachment 623727 [details] [diff] [review]
v1.1

- updated to apply on top of bug 744710
- parallel limit now checked before posting next ProcessNextURI() (that saves a loop over all items)
Comment 8 Michal Novotny (:michal) 2012-05-17 05:11:23 PDT
Comment on attachment 623727 [details] [diff] [review]
v1.1

> +nsOfflineCacheUpdateItem::IsInProgress()
> +{
> +    return mState == nsIDOMLoadStatus::REQUESTED.
> +                  || nsIDOMLoadStatus::RECEIVING;
> +}

This should be

    return mState == nsIDOMLoadStatus::REQUESTED ||
           mState == nsIDOMLoadStatus::RECEIVING;


> +    for (PRUint32 i = 0; i < mItems.Length(); ++i) {
> +        nsOfflineCacheUpdateItem * item = mItems[i];
> +
> +        if (item->IsScheduled()) {
> +            runItem = item;
> +            break;
> +        }
> +
> +        if (item->IsCompleted())
> +            ++completedItems;
> +    }

The item handling could be IMO simplified so that we could have 3 lists (e.g. mItemsQueue, mItemsInProgress, mItemsFinished) and would move the item between the lists according to its state. This is just a proposed optimization, feel free to keep the current code...


> +// Maximim number of parallel items loads

s/Maximim/Maximum/


> +    // Set mState to LOADED here rather then in OnStopRequest to prevent

s/rather then/rather than/


> +    // Accroding parallelism this may imply more pinned retries count,

s/Accroding /According to/


> +    // stop anyway.  Also, this code may soon be completelely removed

s/completelely/completely/
Comment 9 Honza Bambas (:mayhemer) 2012-05-26 05:37:04 PDT
(In reply to Michal Novotny (:michal) from comment #8)
> Comment on attachment 623727 [details] [diff] [review]
> v1.1

> The item handling could be IMO simplified so that we could have 3 lists
> (e.g. mItemsQueue, mItemsInProgress, mItemsFinished) and would move the item
> between the lists according to its state. This is just a proposed
> optimization, feel free to keep the current code...

I'll keep the current.  It's IMO safer and better maintainable.
Comment 10 Honza Bambas (:mayhemer) 2012-05-26 06:52:47 PDT
Created attachment 627480 [details] [diff] [review]
v2

Added tests that should cover most of the code.  Unfortunatelly there is no way to trigger the IsInProgress method.

https://tbpl.mozilla.org/?tree=Try&rev=b329dd2a1bff
Comment 11 Michal Novotny (:michal) 2012-05-29 04:49:41 PDT
Comment on attachment 627480 [details] [diff] [review]
v2

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

There are still few typos in the patch. r+ with those fixed

::: dom/tests/mochitest/ajax/offline/744719-cancel.cacheManifest
@@ +2,5 @@
> +
> +http://mochi.test:8888/tests/SimpleTest/SimpleTest.js
> +http://mochi.test:8888/tests/dom/tests/mochitest/ajax/offline/offlineTests.js
> +
> +# more then 15 what is a number of parallel loads

more than

::: dom/tests/mochitest/ajax/offline/744719.cacheManifest
@@ +2,5 @@
> +
> +http://mochi.test:8888/tests/SimpleTest/SimpleTest.js
> +http://mochi.test:8888/tests/dom/tests/mochitest/ajax/offline/offlineTests.js
> +
> +# more then 15 what is a number of parallel loads

more than

::: dom/tests/mochitest/ajax/offline/test_bug744719-cancel.html
@@ +16,5 @@
> +
> +ok(applicationCache.mozItems.length == 0,
> +   "applicationCache.mozItems should be available and empty before associating with a cache.");
> +
> +function updateCaceled()

updateCanceled

@@ +61,5 @@
> +  OfflineTest.finish();
> +}
> +
> +if (OfflineTest.setup()) {
> +  // Wait some time after the update has been canceled to catch potentiall leaks of channels that would cause

potential

::: uriloader/prefetch/nsOfflineCacheUpdate.cpp
@@ +1474,5 @@
>              return;
>          }
>      }
>  
> +    // Accroding to parallelism this may imply more pinned retries count,

According
Comment 13 Phil Ringnalda (:philor) 2012-05-29 21:12:02 PDT
Comment on attachment 627480 [details] [diff] [review]
v2

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/96fd84dcc396 - the two test failures in https://tbpl.mozilla.org/php/getParsedLog.php?id=12174379&tree=Mozilla-Inbound and https://tbpl.mozilla.org/php/getParsedLog.php?id=12172931&tree=Mozilla-Inbound were maybe survivable, I was filing an intermittent orange bug on them, but the crash in https://tbpl.mozilla.org/php/getParsedLog.php?id=12177269&tree=Mozilla-Inbound tipped me over into backing it out instead.
Comment 14 Honza Bambas (:mayhemer) 2012-05-30 03:22:11 PDT
Thanks Phil.

According the crash, apparently we need to a) prevent call of Finish[NoNotify]() more then one time (just prevent when state is STATE_FINISHED) and b) make items keep the update hard-reffed between successful call to AsyncOpen and after notify of LoadCompleted.
Comment 15 Honza Bambas (:mayhemer) 2012-05-30 04:41:57 PDT
Created attachment 628310 [details] [diff] [review]
v3

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

- items hard-ref the update
- prevent of duplicate call to Finish()
- fixed the js test code
Comment 16 Honza Bambas (:mayhemer) 2012-05-30 04:42:17 PDT
Created attachment 628311 [details] [diff] [review]
interdiff v2->v3

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