Closed Bug 1339248 Opened 3 years ago Closed 3 years ago

Implement a configurable throttle for max concurrent feeds in progress

Categories

(MailNews Core :: Feed Reader, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 55.0

People

(Reporter: alta88, Assigned: alta88)

Details

Attachments

(2 files, 5 obsolete files)

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
Attached patch throttle.patch (obsolete) — Splinter Review
Assignee: nobody → alta88
Attachment #8836918 - Flags: review?(mkmelin+mozilla)
Attached patch unread.patch (obsolete) — Splinter Review
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.
Attachment #8836918 - Attachment is obsolete: true
Attachment #8836918 - Flags: review?(mkmelin+mozilla)
Attachment #8838697 - Flags: review?(mkmelin+mozilla)
Attached patch throttle.patch (obsolete) — Splinter Review
correct patch..
Attachment #8838697 - Attachment is obsolete: true
Attachment #8838697 - Flags: review?(mkmelin+mozilla)
Attachment #8838699 - Flags: review?(mkmelin+mozilla)
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.
Attachment #8838699 - Flags: review?(mkmelin+mozilla)
Attached patch throttle.patch (obsolete) — Splinter Review
This should do it.
Attachment #8838699 - Attachment is obsolete: true
Attachment #8839217 - Flags: review?(mkmelin+mozilla)
Attached patch throttle2.patch (obsolete) — Splinter Review
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.
Attachment #8840063 - Flags: review?(mkmelin+mozilla)
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?
Attachment #8839217 - Flags: review?(mkmelin+mozilla) → review+
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
Attachment #8840063 - Flags: review?(mkmelin+mozilla) → review+
(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.
Attached patch throttle.patchSplinter Review
upated.
Attachment #8839217 - Attachment is obsolete: true
Attachment #8844322 - Flags: review+
Attached patch throttle2.patchSplinter Review
updated for comments.
Attachment #8840063 - Attachment is obsolete: true
Attachment #8844323 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/848dc0659f1a243d251428a65f80b9f135f05d7c
https://hg.mozilla.org/comm-central/rev/e3fa3b9f3d65c86ddeb67865ed821f79a261f2a9
Status: NEW → RESOLVED
Closed: 3 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 55.0
You need to log in before you can comment on or make changes to this bug.