Closed Bug 471208 Opened 11 years ago Closed 11 years ago

Auto-Sync improvement: Using main window's Focus/Unfocus events as TB idle

Categories

(MailNews Core :: Backend, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 3.0b3

People

(Reporter: bugmil.ebirol, Assigned: Bienvenu)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

(Whiteboard: [b3ux][m2][needs updated patch])

Attachments

(2 files, 3 obsolete files)

Background:

At present, Auto-Sync manager uses Idle service to start downloading the pending messages. Unfortunately idle service is system-wide and only kicks in when the computer is not in use. We can improve Auto-Sync mechanism by continue downloading  when TB is not in use.

Proposed improvement:

We introduce Sync-Level concept to auto-sync, something similar to what Gloda uses. 
- Level 0 is System-idle and the current rules apply. 
- Level 1 is TB-idle and it makes auto-sync download messages smaller than certain size (for example 50K). This is a very aggressive mode and its sole purpose is to download as much small messages as possible without needing to wait for the system-idle.

How:

The idea is to use TB's main window's focus/unfocus events as TB idle (thx to Bryan). Auto-Sync hooks into focus/unfocus notification of the main window and when notified, it starts working in level 1 until it gets an idle notification (trigger to switch to level 0), or it reaches the end of the queue. This requires us to introduce a 'Sync Level' concept to the current design. Each level gets its own folder queue and message queue iterator. This prevents us from creating an expensive message queue for each level. Each iterator consists of an offset, a seeking rule (i.e. move to next <50K message etc..) and an error handling scheme.

* Since this approach uses the main window's focus/unfocus events, composing a new e-mail also puts auto-sync to level 1, which is OK. Let me know if you can think of a collusion scenario.
Assignee: nobody → bugmil.ebirol
Completed. Partially tested. Need more test.

If anyone is interested, this patch can be tested after applying patch 469053. Currently there are couple bugs related to newly introduced sync-levels, but this is mainly because bug 469053 doesn't have any sync-level notion yet.

This patch adds the following features:

1) Sync-Levels: SystemIdle, TBIdle, and UserMode 
2) Ability to start/stop auto-sync explicitly for extension developers (I know there is at least one extension under development needs this feature)
I think you have solved most of the implementation issues for on-demand sync (bug 470624) so I'll build my patch on this one.

I've been making quite slow progress as it is (I'm quite rusty with C++ and am still getting my head around XPCOM), but what is really killing me is the time it takes to build. What is the quickest way to build after touching nsAutoSyncManager.*?
It is enough to do 'make' under the <your-objdir-as-defined-in-.mozconfig>/debug/mailnews/imap path.

This will compile only imap related code and link it.
Flags: wanted-thunderbird3+
Blocks: 470624
I'm stealing from emre to keep on my driving radar - who finishes the patch is still TBD.
Assignee: bugmil.ebirol → david.ascher
Depends on: 469053
This one uses new bug 469053 patch as base. It fixes couple problems I have noticed with rev 1.

I have tested until I got "Lock down in sector 4" alert from gmail. It looks good. If somebody can test it would be great. If it works as expected, patch can be reviewed.
Attachment #355231 - Attachment is obsolete: true
Depends on: activitymgr
Marking as blocking, as I think it's pretty important that autosync be as rational/efficient as possible.  We may want to tweak this bug or create a new bug to also leverage the Stopwatch based stuff that asuth has done for gloda (bug 470329).
Flags: blocking-thunderbird3+
Target Milestone: --- → Thunderbird 3.0b3
Do you want me to take this over, David? If so, please just assign to me.
yes, makes sense.
Assignee: david.ascher → bienvenu
Whiteboard: [b3ux]
One thing I'll do is make a generic notification that things like gloda can tie into on app idle, and have auto sync listen for that, instead of having mailWindow.js directly poke autosync.  I suspect I'd like to delay the notification of app idle for a few seconds, so that we can avoid multiple notifications (e.g., don't send any notifications if the focus shifts away and back quickly), and also give the other app an initial clear shot at the cpu - there's no particular hurry if we're really app idle.
I'm going to try for this by m2.
Whiteboard: [b3ux] → [b3ux][m2]
Attached patch wip (obsolete) — Splinter Review
this is a wip patch - it seems to work relatively well. It uses a js module that mailWindows load to keep track of focus events, and turn those into observer service notifications when the app is idle. I changed the autosync code to have the concept of appIdle and systemIdle so that we don't stop autosync when going from systemIdle back to appIdle, for example. This patch doesn't do anything differently between app idle and system idle, and I'm not at all convinced that we really need to right now - we're limited by how fast the imap server can spit data at us, and we can't bring the app to its knees the way gloda can.
Attachment #358802 - Attachment is obsolete: true
asking for r= on TB-specific stuff, r/sr on AutoSyncManager changes.

I've add a js module appIdleManager.js to TB, that mail windows can use to send blur and focus notifications to. The appIdleManager will then turn those into observer service notifications mail:appIdle, "idle" or "back". I also changed autosync to have a concept of appIdle, where when it's app idle it basically ignores system idle notifications. I don't do anything different during app idle for autosync since autosync isn't horribly cpu-intensive in general. If it turns out we need to do different things during appIdle, then we can extend this.

Right now only the 3-pane and stand-alone message window take us out of appIdle - we could make other windows like the compose window also take us out of app idle by having it register for focus and blur events, and passing them along to the appIdleManager.

SM could have its own version of the appIdleManager if it wanted, but it's a bit different what with having a browser and all, so I'll leave that up to them. If it generates the mail:appIdle notifications, autosync will pay attention to them.
Attachment #369403 - Attachment is obsolete: true
Attachment #369527 - Flags: superreview?(bugzilla)
Attachment #369527 - Flags: review?(bugzilla)
Whiteboard: [b3ux][m2] → [b3ux][m2][has patch for review]
Did you miss a file, where does appIdleManager.js get loaded?
yes, sorry, forgot about this directory.
Attachment #369582 - Flags: review?(bugzilla)
pinging for review - I've put this in m2, so I'd like to make some progress on it...
Whiteboard: [b3ux][m2][has patch for review] → [b3ux][m2][has patch for review][needs review Standard]
Comment on attachment 369527 [details] [diff] [review]
make autosync work during app idle

>+var appIdleManager =
>+{
>+  _appIdle: false,
>+  _timer: null,
>+  _timerInterval: 5000, // 5 seconds ought to be plenty
>+  init: function()
>+  {
>+    if (!this._timer)
>+      this._timer = Cc["@mozilla.org/timer;1"].createInstance(Ci.nsITimer);
>+  },

Slight simplification which I'm growing to prefer: Replace init and _timer with:

get _timer() {
  delete this._timer;
  return this._timer = Cc["@mozilla.org/timer;1"].createInstance(Ci.nsITimer);
}

That way we don't need to call init every time, and timer gets initialised on first use.

>   nsCOMPtr<nsIObserverService> observerService =
>          do_GetService("@mozilla.org/observer-service;1", &rv);
>          
>   rv = observerService->AddObserver(this,
>                                     NS_XPCOM_SHUTDOWN_OBSERVER_ID,
>                                     PR_FALSE);
>+  observerService->AddObserver(this, kAppIdleNotification, PR_FALSE);
> }

You need to remove this additional observer when the xpcom-shutdown notification is received, otherwise we could leak memory.

>+  else if (!PL_strcmp(aTopic, kAppIdleNotification))
>+  {
>+    if (nsDependentString(aSomeData).EqualsLiteral("idle"))
>+    {
>+      IdleState prevIdleState = GetIdleState();
>+      

nit: whitespace on empty line (3 places in this function).

>+    // if we're app idle when we get back from system idle, we ignore
>+    // it, since we'll keep doing our idle stuff.
>+    if (GetIdleState() != appIdle)
>+    {
>+      SetIdleState(notIdle);
>+      NOTIFY_LISTENERS(OnStateChanged, (PR_FALSE));      

nit: whitespace on end of line.

r/sr=me with those fixed.
Attachment #369527 - Flags: superreview?(bugzilla)
Attachment #369527 - Flags: superreview+
Attachment #369527 - Flags: review?(bugzilla)
Attachment #369527 - Flags: review+
Attachment #369582 - Flags: review?(bugzilla) → review+
Whiteboard: [b3ux][m2][has patch for review][needs review Standard] → [b3ux][m2][needs updated patch]
Fix checked in, with nits addressed.

Here's how I test that it's working:

add the following string pref

autosyncActivities.logging.dump, All (or console instead of dump if you want output to the console)

Run TB and look for auto sync activities on the console. If the 3-pane or stand-alone message window has focus, and you're not idle for > 10 seconds, you shouldn't see any autosync activity. If you bring an other window or app forward, auto sync should start in 5 seconds are so, independent of whether you're idle or not. Bringing the 3-pane or stand-alone msg window forward should stop autosync activities, until you're idle.

Other TB windows (e.g., compose, address book) are treated now as if they belong to an other app - i.e., autosync happens when they have focus. We're mainly trying to keep autosync from getting in the way of the user doing stuff with the imap server, and that doesn't happen, generally, from the compose or address book windows. We may revisit this, especially for the compose window. That would just entail having the compose window js load the appIdleModule and adding event listeners for blur and focus, just like we did to mailWindow.js
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.