613184 - IMAP protocol causes tens of wakeups

Status: RESOLVED FIXED

(MailNews Core :: Networking: IMAP, defect)

Description

[Cristian KLEIN]

User-Agent:       Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.2.12) Gecko/20101027 Ubuntu/10.10 (maverick) Firefox/3.6.12
Build Identifier: Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.2.12) 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.)

Comment 1

[Cristian KLEIN]

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

Comment 2

[Ludovic Hirlimann [:Usul]]

(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 ?

Comment 3

[Cristian KLEIN]

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 [1].


[1] https://developer.mozilla.org/en/Thunderbird/Thunderbird_MozMill_Testing#Known_Linux_Failures

Comment 4

[Mark Banner (:standard8)]

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

Comment 5

[Mark Banner (:standard8)]

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.

Comment 6

[Cristian KLEIN]

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.

Comment 7

[David :Bienvenu]

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.

Comment 8

[Cristian KLEIN]

Hello,

Could you tell me what additional steps are required to check this patch in?

Thanks,
Cristi.

Comment 9

[rsx11m]

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 10

[David :Bienvenu]

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.

Comment 11

[Cristian KLEIN]

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

[rsx11m]

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.

Comment 13

[David :Bienvenu]

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

Comment 14

[David :Bienvenu]

fixed on trunk:

changeset:   6901:cd4f0d33d6dd
tag:         tip
user:        Christian Klein <[e-mail]>
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.

Comment 15

[Cristian KLEIN]

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.

Comment 16

[Mark Banner (:standard8)]

(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.

Comment 17

[David :Bienvenu]

(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.

Comment 18

[Wayne Mery (:wsmwk)]

(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.

Comment 19

[Cristian KLEIN]

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?

Comment 20

[Wayne Mery (:wsmwk)]

(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.

Comment 21

[Cristian KLEIN]

I'm unable to reproduce the problem by manually disconnecting from the network. Do you happen to know a good network impairer for Linux?

Comment 22

[Joshua Cranmer [:jcranmer]]

(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.

Comment 23

[Wayne Mery (:wsmwk)]

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?