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.

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.

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.

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

Duh, yes, typed that in the wrong tab here. :( Ignore comment 2.

This blocks our webapps story, so blocking kilimanjaro.

Needs more testing (I'll create test just for this new code), but so far seems to be working well.

- updated to apply on top of bug 744710 - parallel limit now checked before posting next ProcessNextURI() (that saves a loop over all items)

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/

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

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 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 on attachment 627480 [details] [diff] [review] v2 https://hg.mozilla.org/integration/mozilla-inbound/rev/e4823da049f8

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.

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.

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 on attachment 628310 [details] [diff] [review] v3 https://hg.mozilla.org/mozilla-central/rev/f2b089df69b3