1339248 - Implement a configurable throttle for max concurrent feeds in progress - preference rss.max_concurrent_feeds

Status: RESOLVED FIXED

(MailNews Core :: Feed Reader, defect)

Description

[alta88]

After Bug 1312813, feed update times can be set individually and spread out so processing can be fairly imperceptible. In the case of startup after a long period of inactivity, it is possible/probable that all update times have expired and all feed files have updates, leading to lots of load for profiles with very many feeds.

This patch does the following:
1. Implement user pref to limit concurrent in progress feeds per poll cycle; feed that didn't make it get picked up in the next cycle in 1 minute, and etc.
2. Tweak to allow really large feeds to not time out.
3. Implement user pref for how long stale feed items should be cached (dupe prevention, default has always been 1 day).
4. Some logging tweaks.

This patch does not:
1. Consider starvation caused user footguns like setting all feeds to update every minute and then the feed does change that frequently (or the publisher doesn't support If-Modified-Since).
2. Consider file size; unfortunately many/most xhr responses do not contain the Content-Length header.

An extreme feed torture test is livejournal, an rdf file over 2MB large, that has over 300 new items every minute. The freeze is caused by gloda indexing that many new messages at once..
http://www.livejournal.com/stats/latest-rss.bml

Comment 1

[alta88]

Attachment: throttle.patch

Comment 2

[alta88]

Attachment: unread.patch

Currently, feeds that return an error are retried in the next cycle, and forever unless fixed or paused. This leads to opening the msgDatabase, resolving an error, and then setting msgDatabase = null to free it; this does not actually gc the memory (seems like a bug). The patch is updated to auto disable (pause) an error state feed. The user can manually get messages on the folder, or go to Subscribe and Update the feed to verify it.

Comment 3

[alta88]

Attachment: throttle.patch

correct patch..

Comment 4

[alta88]

Comment on attachment 8838699 [details] [diff] [review]
throttle.patch

Hmm, need to be more selective when pausing errors, otherwise someone with 300 feeds will have them all paused if there's a general network connectivity problem.

Comment 5

[alta88]

Attachment: throttle.patch

This should do it.

Comment 6

[alta88]

Attachment: throttle2.patch

Part 2:
In order to avoid opening msgDatabase for folders that may not need it, move the test later in the process and after a successful download and the presence of potential items to store. In this case, if a reparse is necessary, use a listener to process the results after reparse, so to avoid another download.

Comment 7

[Magnus Melin [:mkmelin]]

Comment on attachment 8839217 [details] [diff] [review]
throttle.patch

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

::: mailnews/extensions/newsblog/content/Feed.js
@@ +191,5 @@
>        return;
>      }
>  
> +    FeedUtils.log.debug("Feed.onDownloaded: got a download, fileSize:url - " +
> +                        aEvent.loaded + " : " + url);

I'd prefer style like
...    fileSize=" + aEvent.loaded + ", url=" + url);

::: mailnews/extensions/newsblog/content/FeedUtils.jsm
@@ +248,5 @@
>    downloadFeed: function(aFolder, aUrlListener, aIsBiff, aMsgWindow) {
>      FeedUtils.log.debug("downloadFeed: account loginAtStartUp:isBiff:isOffline - " +
>                          aFolder.server.loginAtStartUp + " : " +
>                          aIsBiff + " : " + Services.io.offline);
> +    // User set.

Well it can be automatic too.

@@ +321,5 @@
>  
>          // We need to kick off a download for each feed.
>          let now = Date.now();
>          let msgDbOk = false;
> +        let feedResource, feed;

can we move declarations down to where these are first used please?

Comment 8

[Magnus Melin [:mkmelin]]

Comment on attachment 8840063 [details] [diff] [review]
throttle2.patch

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

LGTM, r=mkmelin

::: mailnews/extensions/newsblog/content/Feed.js
@@ +293,5 @@
> +  // nsIUrlListener methods for getDatabaseWithReparse().
> +  OnStartRunningUrl: function(aUrl) { },
> +  OnStopRunningUrl: function(aUrl, aExitCode)
> +  {
> +    if (Components.isSuccessCode(aExitCode))

please add curly braces for the if and the else
(especially since they are multiline)

@@ +465,5 @@
> +    // At this point, if we have items to potentially store and an existing
> +    // folder, ensure the folder's msgDatabase is openable for new message
> +    // processing. If not, reparse with an async nsIUrlListener |this| to
> +    // continue once the reparse is complete.
> +    if (this.itemsToStore && this.itemsToStore.length && this.folder &&

nit: nicer to instead of this.itemsToStore.length write it
  this.itemsToStore.length > 0

Comment 9

[alta88]

(In reply to Magnus Melin from comment #7)
> Comment on attachment 8839217 [details] [diff] [review]
> throttle.patch
> 
> Review of attachment 8839217 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mailnews/extensions/newsblog/content/Feed.js
> @@ +191,5 @@
> >        return;
> >      }
> >  
> > +    FeedUtils.log.debug("Feed.onDownloaded: got a download, fileSize:url - " +
> > +                        aEvent.loaded + " : " + url);
> 
> I'd prefer style like
> ...    fileSize=" + aEvent.loaded + ", url=" + url);
> 

I have a code cleanup patch queued, one of the items there is to consolidate
the debug logging (likely using console.table) as it's too hard to keep track
currently, so will leave til then.

> ::: mailnews/extensions/newsblog/content/FeedUtils.jsm
> @@ +248,5 @@
> >    downloadFeed: function(aFolder, aUrlListener, aIsBiff, aMsgWindow) {
> >      FeedUtils.log.debug("downloadFeed: account loginAtStartUp:isBiff:isOffline - " +
> >                          aFolder.server.loginAtStartUp + " : " +
> >                          aIsBiff + " : " + Services.io.offline);
> > +    // User set.
> 
> Well it can be automatic too.

I believe Services.io.offline is user pref, ie io.connectivity can be false
(auto detected) and io.offline is still false. 
> 
> @@ +321,5 @@
> >  
> >          // We need to kick off a download for each feed.
> >          let now = Date.now();
> >          let msgDbOk = false;
> > +        let feedResource, feed;
> 
> can we move declarations down to where these are first used please?

ok.

Comment 10

[alta88]

Attachment: throttle.patch

upated.

Comment 11

[alta88]

Attachment: throttle2.patch

updated for comments.

Comment 12

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/comm-central/rev/848dc0659f1a243d251428a65f80b9f135f05d7c
https://hg.mozilla.org/comm-central/rev/e3fa3b9f3d65c86ddeb67865ed821f79a261f2a9