GNOME Bugzilla – Bug 220672
Excessive autosaving uses lots of resources
Last modified: 2010-05-20 09:42:02 UTC
Package: Evolution Priority: Normal Version: 1.0.2 Synopsis: Autosaving uses lots of resources Bugzilla-Product: Evolution Bugzilla-Component: Mailer Description: When messages being composed are autosaved, the CPU gets quite hogged for a while, which is annoying when playing games, watching videos etc. Would it be possible to detect whether anything has been changed since the last autosave and not do anything if nothing has?
I couldn't reproduce this neither find any related report. Moving to 'New' in order to allow developers to ask smarter questions than me.
don't leave a message composer sitting around whilst you watch movies? :-) It shouldn't use that much CPU at all, mostly disk i/o unless you have large attachments that we have to encode or something. anyways, I guess we could. We need a way for gtkhtml to tell us it's dirty bit is set though. anyways, not a high priority right now...
> don't leave a message composer sitting around whilst you watch movies? > :-) Well, the composer windows as a mail queue are just too handy. :) > It shouldn't use that much CPU at all, mostly disk i/o unless you > have large attachments that we have to encode or something. You're right, it's probably the disk I/O which hurts most, but there's also a CPU spike. > anyways, I guess we could. We need a way for gtkhtml to tell us > it's dirty bit is set though. > > anyways, not a high priority right now... This bug probably depends on #948, right? That one is fixed on HEAD, does that mean I have to wait for 1.2? I was hoping to look into fixing this one on the 1.0 branch when I get time...
hmmm, yea. It won't be possible to fix in the 1.0.x branch because the GtkHTML editor doesn't have a dirty bit flag. I don't know how 948 got marked as fixed, since it's kinda not. I guess it was assumed that it was fixed because it works sometimes since it judges dirtyness absed on whether or not the editor has an undo queue, but that is really a horrible way to do it. What if I save with an undo queue? that doesn't erase my undo queue, or at least I should hope not and there for it will remain "dirty" even if I don't touch it again. *** This bug has been marked as a duplicate of 200948 ***
ack, I put 948 in the wrong field :-) I hate it when I do that.
So, will this likely be done properly for 1.2, or should I get my hands dirty? How bad is HEAD, anyway? :)
actually I was looking at the code and I think that this will be solved "for free" when the other bug is fixed. I just talked to Radek and he is going to implement the PersistStreamInterface::dirty_bit stuff. Once that's done it will be easy to fix: edit e_msg_composer_is_dirty() and change the if-statement to check for the dirty bit and not the undo queue and it will be fixed for both closing an edited composer and for auto-save (or if not for auto-save, just add a e_msg_composer_is_dirty() check in the auto-save code).
closing since fixing the other bug will also fix this.
Created attachment 41783 [details] [review] gtkhtml patch
Created attachment 41784 [details] [review] evolution patch
So I sat down and fixed this for good. Messages only seem to get autosaved exactly when necessary now. Please apply; unfortunately, I didn't see a way to do it without adding stuff to gtkhtml, but that shouldn't be a problem?
Michel: can you send your patch to patches@ximian.com for review?
I happily would, but... patches@ximian.com SMTP error from remote mailer after RCPT TO:<patches@ximian.com>: host trna.ximian.com [141.154.95.22]: 550 5.1.1 <patches@ximian.com>... User unknown
Michel: My bad: evolution-patches@ximian.com Sorry for the mistake and thanks
Created attachment 42675 [details] [review] new patch for gtkhtml3.0
Created attachment 42676 [details] [review] new patch for evolution 1.4
the gtkhtml part looks good
for mailer: @@ -2239,6 +2259,9 @@ subject_changed_cb (EMsgComposerHdrs *hd composer = E_MSG_COMPOSER (data); gtk_window_set_title (GTK_WINDOW (composer), subject[0] ? subject : _("Compose a message")); + + /* Mark the composer as changed so it prompts about unsaved changes on close */ + e_msg_composer_set_changed (composer); } that is unneeded because the hdrs-changed signal is also emitted when subject-changed is emitted, so the hdrs_changed_cb() handles this case already. also, I don't understand why you added has_changed_autosave and the new gtkhtml command at all. I don't see how this would fix your "autosaving spikes the CPU" bug at all. Please explain. a ChangeLog would also be nice...
ok, I understand how this solves the problem now. and yes, the mailer patch looks good too (except for that one niggle I mentioned above). as discussed on irc, a better solution would involve emitting the hdrs-changed signal when the from menu changes as well, rather than making from_changed_cb set the dirty bits. this way its cleaner.
Updated patches submitted to evolution-patches@lists.ximian.com .
hmmmm, i think this ended up going in and/or something equivalent did? closing assuming so, reopen if not
(In reply to comment #21) > hmmmm, i think this ended up going in and/or something equivalent did? For the record: no. I gave up on pushing for inclusion after trying for at least a year and am currently rebuilding every new version of Evo locally with this applied. :(
Created attachment 144483 [details] [review] Patch against 2.22.3.1 I got tired of maintaining this after 2.22, but it still bugs me that it autosaves every composer message every minute even if there have been no changes. (Not doing so would also have reduced the impact of bug 590747, e.g.)
Still there in 2.28.
Seeing CORBA mentioned in the patch means it's way out of date. Should work fine in 2.28.1, but will double-check.
(In reply to comment #25) > Seeing CORBA mentioned in the patch means it's way out of date. Sure, I got tired of rebasing it yet another time. :} Just attached it for reference. > Should work fine in 2.28.1, but will double-check. Definitely doesn't for me, all ~/.evolution/.evolution-composer.autosave-* files are still updated every minute.
I understand what's going on now. Think I'd like to tackle this by adding undo stack change notification to GtkHtml, so that we can schedule an autosave when something changes instead of having to poll every minute. Probably not something I can fix for 2.28 since it will require new GtkHtml API, but I'll try for 2.30. What's another six months, right?
(In reply to comment #27) > I understand what's going on now. Think I'd like to tackle this by adding undo > stack change notification to GtkHtml, so that we can schedule an autosave when > something changes instead of having to poll every minute. Sounds good. > Probably not something I can fix for 2.28 since it will require new GtkHtml > API, but I'll try for 2.30. What's another six months, right? After all these years, just another drop in the ocean. :} Thank you for tackling it, much appreciated!
Created attachment 157412 [details] [review] proposed gtkhtml patch for gtkhtml; Make the "changed" property of the GtkHTMLEditor and let invoke HTMLEngine "undo-changed" signal, which is later propagated through the editor out of GtkHTML.
Created attachment 157413 [details] [review] proposed evo patch for evolution; Changes on the autosave code using the new GtkHTMLEditor property and notification, as Matt suggested.
Haven't tested this, but the patches look pretty much like what I had in mind. Thanks Milan! Sadly, we missed 2.30 and since there's API changes here, we can only target Evolution 3.0.
After some brief testing, the patches seem to work as expected. Thanks Milan! Not a problem if this only makes it into 3.0, but it would be great if at least the autosave related regressions in 2.29.92.1 (most importantly bug 613902, but also bug 613904 and bug 575242) could be fixed in 2.30.x.
Milan, can this patch get in for 2.31.2?
Oh bad, I'm not CC'ed on the bug, I didn't know I can commit for two months :( Lesson taken: mark patches as "accepted-commit now" or something, thus someone else, before the actual release, can do it. I'm committing right now for 2.31.2.
Created commit da6162b in gtkhtml master (3.31.2+) Created commit 7aa52ff in evo master (2.31.2+)
We also need to update Evolution's minimum GtkHtml requirement since we're using new API. http://git.gnome.org/browse/evolution/commit/?id=a53abe730b9e0d491ae92586862336b226d297bf
Thanks guys! I've been using these patches ever since comment #32 and haven't noticed any regressions due to them. Now if bug 575242 and bug 613902 could be fixed as well, I'd be a happy camper. :)