744719 - Don't download appcache files one file at a time

Status: RESOLVED FIXED

(Core :: Networking: Cache, defect)

Description

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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

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

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

(In reply to Johnny Stenback (:jst, [e-mail]) 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)]

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

Comment 5

[Johnny Stenback (:jst)]

This blocks our webapps story, so blocking kilimanjaro.

Comment 6

[Honza Bambas (:mayhemer)]

Attachment: 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)]

Attachment: 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]]

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

(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)]

Attachment: 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]]

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 12

[Honza Bambas (:mayhemer)]

Comment on attachment 627480 [details] [diff] [review]
v2

https://hg.mozilla.org/integration/mozilla-inbound/rev/e4823da049f8

Comment 13

[Phil Ringnalda (:philor)]

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

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

Attachment: 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)]

Attachment: interdiff v2->v3

Comment 17

[Honza Bambas (:mayhemer)]

Comment on attachment 628310 [details] [diff] [review]
v3

https://hg.mozilla.org/mozilla-central/rev/f2b089df69b3