User-Agent: Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:188.8.131.52) Gecko/20101027 Ubuntu/10.10 (maverick) Firefox/3.6.12 Build Identifier: Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:184.108.40.206) Gecko/20101027 Lightning/1.0b2pre Thunderbird/3.1.6 powertop is a tool on Linux to measure the number of wakeups for each process, i.e. how often the processor went from idle to doing something. This is very important for power consumption. Ideally, thunderbird should have 0 wakeups if it's idle. On my Linux system, an idle thunderbird has around 14 wakeups/s. By debugging this, I found out that each IMAP connection generates at least 1 wakeup/s. Having 3 account * 4 IMAP connection, one has 12 useless wakeups/s. Reproducible: Always Steps to Reproduce: 1. Run thunderbird, with 3 IMAP accounts. 2. Wait for it to go idle (e.g. 30 seconds). 3. Run 'sudo powertop -d -t 5 | grep thunderbird'. Actual Results: Powertop reports 14 wakeups/s Expected Results: Powertop should report as few wakeups as possible. By default, Thunderbird creates 4 IMAP connections / account. Each IMAP connection is handles by a separate thread. The file 'mailnews/imap/src/nsImapProtocol.cpp' defines kImapSleepTime to 1000 ms. This variable is used as a timeout for the wait in the protocol loop on line 1358. Thus, each thread is forced to wake up at least 1 per second. I propose a patch which increases this value to 60 seconds. I have not observed any negative effects. (IMHO, this value should be infinity.)
Created attachment 491507 [details] [diff] [review] Patch against comm-central. This patch increases the timeout of IMAP threads from 1 second to 60 seconds. The number of wakeups is significantly reduced, as can be seen below: Before: $ sudo powertop -d -t 5 | grep thunderbird 4,4% ( 14,0) thunderbird-bin After: $ sudo powertop -d -t 5 | grep thunderbird 1,4% ( 5,4) thunderbird-bin
Status: UNCONFIRMED → ASSIGNED
Component: General → Networking: IMAP
Ever confirmed: true
Product: Thunderbird → MailNews Core
QA Contact: general → networking.imap
Version: 3.1 → Trunk
(In reply to comment #1) > Created attachment 491507 [details] [diff] [review] > Patch against comm-central. > > This patch increases the timeout of IMAP threads from 1 second to 60 seconds. > The number of wakeups is significantly reduced, as can be seen below: Did you ran our unit test when you build the patch ? Were there any failures ?
Could you please tell me which unit test you would like me to run. I was unable to find something IMAP specific. I did not run all tests, since I read that they currently fail on Linux .  https://developer.mozilla.org/en/Thunderbird/Thunderbird_MozMill_Testing#Known_Linux_Failures
You want to run the xpcshell-tests, not the mozmill tests: https://developer.mozilla.org/en/Thunderbird/Thunderbird_Automated_Testing#xpcshell-tests_and_make_check cd objdir && make -C mailnews xpcshell-tests && make -C mail xpcshell-tests
I'm not convinced 60 seconds is a good idea actually, if I understand the code correctly it could cause some significant waits between actions happening if you catch it just wrong. David (who I'm cc'ing) should know more.
All tests passed. If I understand the code correctly, each action should notify a monitor, so no waits should happen. The only reason I see for the timeout is to check for DeathSignalReceived(), which probably happens if a user quits Thunderbird during IMAP activity.
DeathSignalReceived can happen for all sorts of reasons, not just the user quitting Thunderbird, e.g., a connection getting killed. TellThreadToDie does notify the monitor, so this patch is probably OK for everything but the case where we get an error on the mock channel request (in nsImapProtocol::DeathSignalReceived), in which case, we don't notify the monitor, so we would wait 60 seconds.
Hello, Could you tell me what additional steps are required to check this patch in? Thanks, Cristi.
You need approval from a module owner, which is David in this case and pending. Before that can happen, you apparently need to address comment #7 somehow.
Comment on attachment 491507 [details] [diff] [review] Patch against comm-central. minusing per comment 7 - your patch doesn't handle the case described in that comment.
Attachment #491507 - Flags: review?(bienvenu) → review-
Created attachment 495282 [details] [diff] [review] Patch against comm-central, version 2. Hello, This patch is an extension to the previous patch, which hopefully addresses comment 7. The IMAP protocol thread needs to be notified when the status of the mock channel changes from NS_OK, so that DeathSignalReceived() can tell it to exit. This status is stored in the m_cancelStatus attribute of the nsImapMockChannel class, which can only be changed from two places: NotifyStartEndReadFromCache() and Cancel(). The latter already calls imapProtocol->TellThreadToDie(), so I added something similar to the former. The patch passes all xpc-shell tests. Please review the patch. Regards, Cristi.
Comment on attachment 495282 [details] [diff] [review] Patch against comm-central, version 2. You have to specify the e-mail address of the requestee for the review when setting that flag. I'm directing this request to David for you.
Attachment #495282 - Flags: review? → review?(bienvenu)
Created attachment 501037 [details] [diff] [review] whitespace cleanup of prev patch. Thx for the second patch - it looks ok, but I want to make sure the unit tests pass on all platforms. this is what I'll push, if the try server builds I've requested result in passed tests.
fixed on trunk: changeset: 6901:cd4f0d33d6dd tag: tip user: Christian Klein <email@example.com> date: Tue Jan 04 13:30:54 2011 -0800 summary: fix bug 613184, imap threads wake up much more than needed, r=bienv enu If we get a significant increase in the number of reports of hangs on shutdown, we might have to revisit this patch.
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
Hello, Would it be possible to apply this patch against comm-1.9.2. No changes are necessary, as the patch applies cleanly. Thanks, Cristi.
(In reply to comment #15) > Would it be possible to apply this patch against comm-1.9.2. No changes are > necessary, as the patch applies cleanly. Whilst it applies cleanly, comm-1.9.2 is a stable release branch that only gets security and stability fixes, we try to avoid risky patches there. As David is concerned about hangs on shutdown, then I would say this needs at least a couple of months baking before we consider it for comm-1.9.2, though I will flag it to be considered later.
status-thunderbird3.1: --- → ?
(In reply to comment #16) > As David is concerned about hangs on shutdown, then I would say this needs at > least a couple of months baking before we consider it for comm-1.9.2, though I > will flag it to be considered later. Yes, I agree.
(In reply to comment #17) > (In reply to comment #16) > > As David is concerned about hangs on shutdown, then I would say this needs at > > least a couple of months baking before we consider it for comm-1.9.2, though I > > will flag it to be considered later. > > Yes, I agree. Cristian, do you get any shutdown hangs with trunk? I have filed regression Bug 634526 - shutdown hang with imap threads blocked - but it is not yet diagnosed. Nor do I know if it is a regression from this bug.
I'm running Thunderbird 3.1.7 + the patch attached to this bug and I haven't got any shutdown hangs. Then again, I'm rarely shutting down Thunderbird (4 times / day tops) and I use a pretty stable network and good servers. Do you think I should try a nightly build of comm-central?
(In reply to comment #19) > I'm running Thunderbird 3.1.7 + the patch attached to this bug and I haven't > got any shutdown hangs. Then again, I'm rarely shutting down Thunderbird (4 > times / day tops) and I use a pretty stable network and good servers. ditto > Do you think I should try a nightly build of comm-central? yes, if you can. It will be interesting - whether you can reproduce or not reproduce. I almost always get Bug 634526, but pending David's analysis I don't know if it's server related.
I'm unable to reproduce the problem by manually disconnecting from the network. Do you happen to know a good network impairer for Linux?
(In reply to comment #21) > I'm unable to reproduce the problem by manually disconnecting from the network. > Do you happen to know a good network impairer for Linux? The program tc should be able to partially handle your needs; I know I have tested high latency before by delaying packet egress for ~3 seconds or so. It's a lot more powerful than I have time to play with for handling.
bienvenu, or anyone, can this perf problem be *significant* for some users, like for a user with 14 imap accounts with 300k messages (mcepl), such that they would see pauses in the UI?
You need to log in before you can comment on or make changes to this bug.