Closed Bug 435153 Opened 17 years ago Closed 17 years ago

Better Faster IMAP: Pseudo-offline Delete and Move support

Categories

(MailNews Core :: Backend, defect)

Product:
MailNews Core
Component:
Backend
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
mozilla1.9

People

(Reporter: bugmil.ebirol, Assigned: bugmil.ebirol)

References

(
URL
)

Details

Attachments

(3 files, 7 obsolete files)

Starting of the stack when copying messages from a local folder to online imap folder
20.07 KB, text/html
Details
Patch rev 8
13.12 KB, text/plain
Bienvenu
: review+
Details
patch for checkin
12.90 KB, patch
Details | Diff | Splinter Review 
The basic idea is to use the offline capabilities of Thunderbird while online to make the UI more responsive, when operating on IMAP messages, out of the box, without the user having to tweak any settings. Pseudo-offline Delete and Move support is the first feature implementation in this direction.

The idea is to perform message moves and deletes "pseudo-offline", which means we do the operations as if we were offline (storing the operations as offline ops in the db), which is very quick, and then playing back the operations to the imap server later. So, for example, deleting an imap message could change from issuing the protocol to copy the message to the trash, and to store the deleted flag on it, and when that's done, load the next message from the imap server, to storing the move as an offline operation in the db, and then loading the next message from the offline store, and issuing the protocol to copy and delete the message while the user is reading the next message.

Different strategies will be experimented to find out the best playback-time in practice.

See wiki.mozilla.org/MailNews:Better_Faster_IMAP2 more info.
Flags: wanted-thunderbird3.0a2?
Initially I wanted to move this to Core -> Networking: IMAP, but as this is a Thunderbird bug, I'm not so sure anymore..
URL: http://wiki.mozilla.org/MailNews:Bett...
Yeah this is core; imap (and/or backend).
Component: General → Networking: IMAP
Product: Thunderbird → Core
QA Contact: general → networking.imap
Target Milestone: Thunderbird 3 → ---
I think it's really more backend - I don't know if we're going to need to change the imap networking code (e.g., nsImapProtocol.cpp, the parser, etc) at all. I think the imap protocol code probably provides all the functionality we need.
Component: Networking: IMAP → MailNews: Backend
QA Contact: networking.imap → backend
I think most of the changes will be done in nsImapMailFolder, nsImapOfflineSync, and no change is needed in nsImapProtocol.cpp.
*** I make it dependent to bug 435259 ***
Depends on: 435259
Attached file Draft patch 1 (obsolete) — 
This patch is NOT COMPLETE. Its purpose is to start discussing the implementation possibilities, and start experimenting UI.

Issues need to be addressed:

1- Current patch uses a timer to playback pending offline ops. Is there any significant advantage to use this approach over "next message" approach?
   - The only reason I use this approach is the simplicity. I understand that  "next message" approach requires us to hook in at the right spot(s), and handle at least one boundary condition. I made my decision in favor of less code/logic change. David, do you see any flaw with this approach?

2- Current patch doesn't release "PseudoOfflineOpPlaybackRequest" instances. It leaks. How and where we should store and destroy them?
    - Storing them in a central location looks better to me than storing per nsImapMailFolder object. any singleton (service) that can be accessed by nsImapMailFolder sounds reasonable to me. What about nsMsgOfflineManager? Is it used as a service?

3- Current patch doesn't handle error cases where playback operation is failed.
   - I understand that nsImapOfflineSync handles playback operation errors gracefully (shows a error dialog, and doesn't remove the pending ops). The question is when we should repeat this operation after an error, and whether we should show an error dialog to the user again, for each try? David? Byran?

Other problems:

 - How to deal with IDLE command
 - After the move operations, after the destination folder is selected, sometimes, UI fetches all messages from the server again for no apparent reason. 
 - Select all messages in folder X, delete, stay in X folder. It moves the messages from X to Trash but doesn't refresh the content of  X folder. Happened once.

Food for thought:

 - Should we playback all pending operations every time, or should be consider to reduce the playback scope for pseudo-offline ops (i.e. instead traversing all folders, starting directly from the folders in question).
I think the timer approach is definitely worth trying. 4.x would attach a listener to the url that was run to load the next message but that's not so easy in the current code base. The one drawback of the timer approach is that if we're in the middle of loading the next message from the server when the timer fires, the offline op playback url will get queued instead of run directly, and that shows up in the protocol log, and adds a little bit of excitement as to whether the url will actually get run (it should work just fine but...)

Re releasing the playbackrequest object, you could just make it own a reference to itself, and release that reference when it handles the timer firing.

The offline manager is a service, but it's only currently used when the user does an explicit offline playback or download, i.e., by doing file | offline | download/sync now.

> should we consider
> reducing the playback scope for pseudo-offline ops (i.e. instead traversing
> all folders, starting directly from the folders in question).

I would say yes (imagine the case of users with a large number of folders) - you know what the folder that needs offline ops played back is, because you've cleverly stored it in your playback request object. Instead of going through the imap service, you can simply create your own   nsImapOfflineSync(nsIMsgWindow *window, nsIUrlListener *listener, nsIMsgFolder *singleFolderOnly = nsnull);

and pass in the single folder to update to the constructor, and then call ProcessNextOperation.  The code has been designed to work that from its beginnings in 4.x.


>The one drawback of the timer approach is that
>if we're in the middle of loading the next message from the server when the
>timer fires, the offline op playback url will get queued instead of run
>directly, and that shows up in the protocol log, and adds a little bit of
>excitement as to whether the url will actually get run (it should work just
>fine but...)

I thought that imap queue is well tested, well known part of the protocol layer. If this is not the case, maybe we should consider conducting more tests on Q as part of this work, because this is an essential mechanism for  Better Faster Imap work in general. Do you suggest to do more tests for imap command queue mechanism?

>Re releasing the playbackrequest object, you could just make it own a reference
>to itself, and release that reference when it handles the timer firing.

Well, I haven't implemented it as a component. It is definitely not lot of work to convert it to a component but since playbackrequest internal to the system, isn't better to keep it as a regular C++ class, and hide it from the rest of the system?

>you can simply create your own  
>nsImapOfflineSync(nsIMsgWindow *window, nsIUrlListener *listener, nsIMsgFolder
>*singleFolderOnly = nsnull);
>and pass in the single folder to update to the constructor, and then call
>ProcessNextOperation.  The code has been designed to work that from its
>beginnings in 4.x.

Great, going to do it after we dealt with the existing problems.
> I thought that imap queue is well tested

It's certainly well tested, but it's only used in edge conditions. The problem with flooding the url queue with stuff is that you prevent the user from getting a word in edge-wise. If we have 10 imap things to do, we prefer to do them serially, instead of just running 10 urls at once, because if we do them serially, the user only has to wait for the current operation to finish, instead of all 10 to finish, since the queue is done in order.  I think it's OK in this situation, but I would not have a general implementation strategy of running a bunch of urls simultaneously.

> Well, I haven't implemented it as a component. 

You mean you haven't implemented nsISupports and ref counting? In that case, when you're done, you can just do delete this. Except that you're not really done until the operation has succeeded or failed, which happens asynchronously, which means your object really needs to implement nsIUrlListener, to know when the url is done, and whether it succeeded or failed.
having tests for the url queue would be great, but you'll need to be able to fake an imap server, and fake all the interesting things that happen with networks and imap servers, like connections going away in the middle of operations, etc.
I think I misunderstood what you said at comment #7:

>..a little bit of excitement as to whether the url will actually 
> get run (it  should work just fine but...)

I interpreted that there is a know bug or something about imap Qeueu preventing it to function properly when flooded with urls.

>... but I would not have a general implementation strategy of running a bunch of urls simultaneously.

Oh no, I don't suggest that at all. I believe that running URLs serially, as it is, serves us better in this regard. But still not clear to me what you mean by:

>The problem with flooding the url queue with stuff is that you prevent the user from getting a word in edge-wise.

Does it have to do with nsImapOfflineSync's state machine. Can you explain more please.
>I interpreted that there is a know bug or something about imap Qeueu preventing
> it to function properly when flooded with urls.

No known issues anymore, but it was not a lot of fun to get it to a state where there are no known issues :-) 

>The problem with flooding the url queue with stuff is that you prevent the >user from getting a word in edge-wise.

No, this is just a general issue. For example, take the case of analyzing new messages to see if they're spam. This involves fetching them from the server one by one and running them through the spam filter. One way of doing this would be to fire off urls for each new message simultaneously, so if there are 10 new messages, run 10 urls at once, and let the queue serialize the requests.  But if we did that, it's possible that the user couldn't read one of the new messages until all 10 messages had been fetched from the server, because the user's url to fetch a message would get put in the queue after the other 10 urls.

Instead, what we do is chain the urls, so we fetch the first message to see if it's spam, and when that request is done, we fetch the next message, and so on. At any point, the user can click on a message, and it will get queued up right after the current single message fetch, instead of after 10 message fetch urls.

Oh I think I know where the misunderstanding is. I believe you thought that we run urls in the timer event, and handle the url result (via nsIUrlListener) in it. Actually, what the patch does is to call imapService->PlaybackAllOfflineOperations as a whole - so, request is process by nsImapOfflineSync's state machine as usual, and evaluate success and failure returning from this call, not per url. Per url results are handled by this state machine in the existent code afaik. 

Since timers are ran by UI thread, I thought that firing up timers for playbackrequest will also be serialized by UI thread message queue (I think Gecko handles that). Makes sense?

Please let me know if you see any inconsistency in the patch with the explanation above.
Hmm, I assumed that Gecko fires the timer in the same thread always. If it fires them using a thread pool, then it is totally different story.

>You mean you haven't implemented nsISupports and ref counting? In that case,
>when you're done, you can just do delete this.

Do you suggest to call "delete this;" in the object? Is this acceptable by C++ standards? I am not sure. I might be wrong but It looks to me that the result of it is compiler dependent which cause lots of trouble on the long run.
I'm not talking about threads, or thread-safety, or anything complicated like that. Here's the scenario:

I delete an imap message
The next message starts loading from the imap server (if we don't have it cached yet)
Half a second later, we start playing back the offline event to delete the first message.

If the load of the next message takes more than half a second, we will try to run a url to delete the first message (as part of the offline sync), while the message load url is still running. That second url will get queued. It's definitely worth trying your timer approach; I was only saying that the only potential negative of the timer approach is that it will occasionally cause url queueing.

>Do you suggest to call "delete this;" in the object? Is this acceptable by C++
>standards? 
Sure, mailnews does it in several places. XPCom does it in several places. For example, when the ref count for a weak ref object goes to zero :-)
Couple decisions made on IRC:

o Release the playbackrequest instance in the timer callback 
o implement nsIUrlListener interface, and handle errors in the timer callback function
o consider strategies for 3 different type of errors
  - immediate errors returned by imapService->PlaybackAllOfflineOperations method. Mostly standard NSPR errors such as out of memory etc..
  - network errors that will be returned by OnStopRunningUrl callback in exit code parameter
  - server errors that will be returned by OnStopRunningUrl callback in exit code parameter
o consider a new UI notification mechanism for different error types
o consider reducing the scope of the playback operation to a specific imap server and a specific folder.

> consider a new UI notification mechanism for different error types

I'm not convinced that any new error handling mechanism should be specific to offline playback. It should be general to imap operation handling in general, whether we issue the protocol immediately, or half a second later.

I keep getting confused about some assumptions I believe I've been making.  I have been consistently assuming that most mail will be cached such that most user operations wouldn't conflict.

That assumption, of course requires the assumption that we have a good cache strategy.  Meaning we download the appropriate messages at the best times and save them in the cache so they are available for reading and searching any time in the future.  We can have lots of discussion about this strategy because I think it will need to be driven by type of interface we present and the experience we want the user to have so I have some ideas already.

Of course these assumptions will likely incur a worse experience for people who disable the cache, which we would be enabling by default.   We could work on that issue separately from this one. 

Just bringing this up not because I need to understand the caching system as much as I've been wanting to tune the experience around always having messages available for reading.  So reading mail only a blocking operation when our caching strategy failed.   While the rest of the offline operations being discussed are more of an "act on mail" type of action that I see as easier to put in the background.  

I should make a run through of those types of operations and how they interact next.
Bryan, that's all true - it's just that this needs to be designed up front to handle both cached and non-cached cases. You can't go back and fix it for the non-cached case.
Attached patch Draft rev 2 (obsolete) — — Splinter Review 
Did couple changes:
- Added a manager class to keep track of created requests - mostly for IDLE and error handling
- Instead of creating timer in each request, I moved the timer into manager class. If there is pending request, it fires timer to playback, otherwise it stops the timer
- Added mechanism to group request: Mostly for the scenario where the user deletes messages one-by-one, repeatedly. It postpone playback until the user pauses
- It handles IDLE messages belong to the destination folder after the playback is completed
- Added an aging mechanism to prevent unnecessary IDLE message filtering

What is missing:
- Still can't get error messages from imap layer. OnStopRunningUrl doesn't return any error code.
- Updating destination folder silently, immediately after the playback, on background (?)

Problems:
- Empty messages show up when deleting one-by-one, repeatedly
- View index jumps, confuses, when deleting one-by-one, repeatedly
- After move/delete, when the destination folder is selected, it requires to update its content
Attachment #322329 - Attachment is obsolete: true
Problems I have mentioned look very similar to Bug 414723 ?
It causes an infinite look until it crashes. Will take a look at it.
s/look/loop
Tried doing global pseudo-offline playback when the destination folder and the source folder are not on the same server. Result is same.

The recursion in the state machine continues until it runs out of stack. Major repeating pattern is:

#46	0x1160ee04 in nsImapMailFolder::CopyMessages at nsImapMailFolder.cpp:6764
#47	0x1115fb04 in nsMsgCopyService::DoNextCopy at nsMsgCopyService.cpp:323
#48	0x1115fea4 in nsMsgCopyService::DoCopy at nsMsgCopyService.cpp:265
#49	0x11160f55 in nsMsgCopyService::CopyMessages at nsMsgCopyService.cpp:520
#50	0x1167d920 in nsImapOfflineSync::ProcessMoveOperation at nsImapOfflineSync.cpp:567
#51	0x1167f18f in nsImapOfflineSync::ProcessNextOperation at nsImapOfflineSync.cpp:965
#52	0x1167b5d6 in nsImapOfflineSync::OnStopRunningUrl at nsImapOfflineSync.cpp:123
#53	0x1167a6b3 in nsImapOfflineSync::OnStopCopy at nsImapOfflineSync.cpp:1209
#54	0x1115f88c in nsMsgCopyService::ClearRequest at nsMsgCopyService.cpp:217
#55	0x1115fdf8 in nsMsgCopyService::NotifyCompletion at nsMsgCopyService.cpp:656
#56	0x115f8daa in nsImapMailFolder::OnCopyCompleted at nsImapMailFolder.cpp:7524
#57	0x1160ed26 in nsImapMailFolder::CopyMessagesOffline at nsImapMailFolder.cpp:6744

Emre, this is because the offline playback is using the copy service, which uses nsImapMailFolder::CopyMessages, which you have changed to always create an offline operation and playback, which uses the copy service, which...

So, you need to figure out a way to make nsImapMailFolder::CopyMessages know whether or not you're playing back an offline operation, or the user is doing the operation.
Attached patch Patch rev 3 (obsolete) — — Splinter Review 
First proposal patch.
Attachment #323171 - Attachment is obsolete: true
Attachment #323506 - Flags: review?(bienvenu)
Comment on attachment 323506 [details] [diff] [review]
Patch rev 3

Can I get a -uwp8 diff, so that I don't have to review white space changes?

Also, the prevailing braces style is

if ()
{
}
else
{
}|

not K&R style.

should fix this:
+#endif
\ No newline at end of file

I'm not sure why you want to ignore IDLE responses. We're going to want to get the headers eventually, and when we have an IDLE connection to the folder is a good time...
Changes are:
o Used -uw8p when creating the patch
o Braces are changed as suggested
o ending is fixed

>I'm not sure why you want to ignore IDLE responses. We're going to want to get
>the headers eventually, and when we have an IDLE connection to the folder is a
>good time...

As you know, what it does is to ignore the first IDLE message coming from the server right after (timeout 5 secs) the pseudo offline operation playback. When we discuss this behavior last week I got the impression that both of us agreed that IDLE refresh is annoying for the user. Basically that's why I have implemented such mechanism. This is definitely an UX improvement than a technical necessity. 

Also I must add that until the conversation we had yesterday, I had the impression that there is an easy way to silently update the headers either explicitly, or during the select folder. If this is not the case, I guess the user is going to get this noise anyhow, either after IDLE, or after an UI event.

So, do you suggest not to ignore IDLE message after playback?
Attachment #323506 - Attachment is obsolete: true
Attachment #323506 - Flags: review?(bienvenu)
Attachment #323603 - Flags: review?(bienvenu)
> 
> As you know, what it does is to ignore the first IDLE message coming from the
> server right after (timeout 5 secs) the pseudo offline operation playback. 
Right, sorry, I should have said "Ignore the idle response in this case..

> we discuss this behavior last week I got the impression that both of us agreed
> that IDLE refresh is annoying for the user.
Well, it was more of a mystery than an annoyance, in my mind, until we figured out that it was coming from IDLE. I'm not sure it's an annoyance, but I haven't been using this as intensively as you have.
 
> 
> Also I must add that until the conversation we had yesterday, I had the
> impression that there is an easy way to silently update the headers either
> explicitly, or during the select folder. If this is not the case, I guess the
> user is going to get this noise anyhow, either after IDLE, or after an UI
> event.
Yes, we're going to have to get the headers at some point. And prior to any of your work, we would have retrieved the headers, and no one complained about or even noticed that behavior, if that makes sense.
> 
> So, do you suggest not to ignore IDLE message after playback?
> 
I'd like to suggest not ignoring IDLE messages, and if turns out to be an issue, we can always resurrect your code. In general, I prefer keeping in sync with the server as much as possible, unless it is an overall detriment to the UX.

Sorry I wasn't clear before :-(

>I prefer keeping in sync with the server as much as possible, unless it is an 
>overall detriment to the UX.
David, I am definitely with you on this. We should be in sync with the server as much as possible. What I am questioning here I guess  is "can/should we execute this sync silently". Imagine a scenario where the user moves 100 messages from Folder A to Folder B. With Pseudo-offline feature enabled, this operation is completed immediately, that's perfect, but, when she selects the destination folder, she witnesses that the number of messages in the folder goes up to 200, then the listview is starting to refresh its content by removing the first 100 messages, and then the message number goes back to 100 again. if IDLE response is not ignored, this activity happens in couple seconds after the playback independent from the folder selection.

As I understand it, during pseudo-offline operation we add the headers into the database with fake ids since we don't know the actual ids (one of the RFC you are working on is related to this I guess), and during the "sync", the database replaces them with the headers with real ids. And the user observes all this activity unnecessarily - that's why I call it noise/annoyance. So, from the technical point of view this definitely makes sense, but from the user perspective, I think it's confusing - but this is just me.

So, assuming that I understand the behavior and its rationale correctly, I am asking this question: "is it possible to change just the ids instead of removing/adding headers - at least for the headers carrying same information?". If so, how much effort required to implement? Having said that since this happens in the database, unless the design does explicitly allows it, it shouldn't be an easy task, and I think you have already confirmed that.

Since this bug is about improving IMAP UX, I think it is fair to ask these questions. Thoughts?


it's totally fair to ask those questions. You're right that, ideally, the user wouldn't see the 100 messages get deleted, and then 100 messages get added. Instead, we would still download the 100 headers, but just replace the fake headers with the real headers, and tweak the view, replacing all the fake keys with real keys. That's doable, I think, but not easy. I'd have to think about how I would implement that. One general approach would be to have the view ignore the removal of fake keys, and the addition of new keys, and instead tell it to replace one key with an other key. But that's a pretty half-baked thought at this point. An other approach would be tell the view about a set of message-ids that this is going to happen to - then, when it gets told about a delete of message with that message-id, it would ignore it, and when it gets told about the addition of a message with that same message id, it would go find the old message with that message id, and tweak that view entry to have the new message key.
One advantage of paying attention to the idle responses is that we will more often update folders that aren't visible to the user, so they will see this delete and add *less* often, since it will happen behind the scenes.
Comment on attachment 323603 [details] [diff] [review]
Patch rev 4: is changed as suggested

I'd like to try this without the disabling of IDLE - I believe handling IDLE responses will actually improve the user experience when the user selects the folder, because we will have already downloaded the new headers.
Attachment #323603 - Flags: review?(bienvenu) → review-
Can I assume that it is ok except IDLE handling? Do you have any comments about the questions I asked in the code?

Easy way to bypass "IDLE disabling" is to set the RETIREMENT_AGE parameter to 0. That's way we can keep the mechanism. Are you ok with that?

thanks
I'd rather just remove the code - I don't think we're going to use it, so it would just be sitting there, complicating code that doesn't need to be any more complicated...it seems like you could remove a lot of code if you got rid of the aging stuff. I'd like to see what's left after removing that.

+        switch ( GetState() )
+        {
+          case CompletedOrCanceled: return mOpResult;
+          case Error: return Failure;
+          default:
+            return NoResultYet;

we usually put returns on their own lines

+      //XXX ebirol for some reason I don't see this method is called. 
+      // so, I don't put any state transition logic in it.

If OnStartRunningUrl isn't getting called, that implies a bug either in the listener getting attached, or the notifications getting sent.

+          //XXX ebirol error handling code goes here!
+          //Reviewer, exitCode is always 0, please advise

I'm going to have to debug that to see why that's the case. But I'd rather wait until the patch is closer to being done before applying it...


In general, we assume that code is only ever called from the UI thread, and it's not thread-safe. It's much easier to note the exceptions of code that is called from the imap thread, and/or is thread safe.

No code in nsImapMailFolder.cpp is ever called from anything but the UI thread. I believe only code in nsImapProtocol.cpp, the two parser classes, and the host session list are ever called from a non-UI thread. If we were to comment on all the code that wasn't thread-safe, or assert that we were on the UI thread, we could literally add 10's of thousands of comments and assertions throughout the code. So it might be simpler just to add a big comment at the top of your file saying that it's only ever used on the UI thread. In general, if we ever mess up and call things from the wrong thread, we get a ton of nsISupports assertions anyway.

+        //XXX ebirol does nsTArray's enumerator allow me
+        //to remove in the loop. I guess it does but couldn't be sure.
+        //Reviewer please advise.

I believe you're doing the right thing by removing from the end.

+    } gPseudoOffPlybckMgr;  // TU-wide instance

does that mean TB? It's not TB specific, since SM could use it too.

Where does nsPseudoOfflinePlaybackManager get created? And why does it need a copy constructor? I couldn't find it in the code. In general, we don't like to create global c++ objects like that - we use services which we know will get cleaned up properly. But that might be over xpcom-ificaion. 

wanted‑thunderbird3.0a2+; best to get the ball rolling...
Flags: wanted-thunderbird3.0a2? → wanted-thunderbird3.0a2+
>I'd rather just remove the code - I don't think we're going to use it,
I did remove the IDLE handling code, but prefer to keep the aging mechanism since I am planning to use the same code for phase 2 (preemptive message download). Not sure what will be an application right now, but it is better to keep it until we are sure that there is no use for it. 


>we usually put returns on their own lines
done

>If OnStartRunningUrl isn't getting called, that implies a bug either in the
>listener getting attached, or the notifications getting sent.

>I'm going to have to debug that to see why that's the case. But I'd rather wait
>until the patch is closer to being done before applying it...
Waiting feedback from you.

>In general, we assume that code is only ever called from the UI thread, 
done

>+    } gPseudoOffPlybckMgr;  // TU-wide instance
>does that mean TB? It's not TB specific, since SM could use it too.
No, it means Translation Unit. This is a similar technique to what 'static' does to functions in C. gPseudoOffPlybckMgr is global but can only be accessed by the cpp file includes it - that is nsImapMailFolder in our case.

>And why does it need a copy constructor? I couldn't find it in the code.
It doesn't, that's the point. This declaration prevents compiler from generating implicit copy ctor.

>In general, we don't like to create global c++ objects like that - we use >services which we know will get cleaned up properly. But that might be over >xpcom-ificaion. 
Since this class won't be used by other part of the system, I thought it's better to hide it from the rest of the system. And, based on the new deCOMtamination initiative, I preferred a simple C++ class instead of a service. One alternative would be to implement it as a singleton, but even that would be overkill I guess.

I have couple questions need your attention in the code, especially the one about nsImapOfflineSync creation is important since it might cause a memory leak.
Attachment #323603 - Attachment is obsolete: true
Attachment #324331 - Flags: review?(bienvenu)
> Not sure what will be an application right now, but it is better to
> keep it until we are sure that there is no use for it. 

We do it the other way around - unless we have a pretty good idea that the code's going to be used, we don't add it to the cvs repository.  Otherwise, we have to spend more time taking it out later. And I don't think you'd be using the pseudooffline playback manager for preemptive message downloading so the code would have to move in any case...the automatic downloading process seems fairly separate from pseudo-offline playback. (and as a side note, for a class that big, we tend to put the class declaration in the .h file and the implementation in a .cpp file) 

I'm just not convinced that this extra class is needed at all for pseudo-offline playback. From what I can tell, all it really does is prevent us from doing multiple offline playbacks in the case where the user does multiple deletes in less than half a second (it would be very difficult for the user to do multiple moves in the same space of time). I think you wanted to add error handling to this global manager, but it seems to me that any special error handling beyond the normal error handling for imap failures should be done by the already existing offline playback mechanism, instead of inventing a new pseudo-offline manager.

I can't tell how much of the reason you made this new class is because stuff is going to get added to it eventually, and how much of it is because you weren't comfortable with your knowledge of how the existing code works and felt it was easier to control things with a new layer of code. If you could elaborate on that, I might understand the need for this whole new layer of code - to me it seems to add a whole lot more places the code could fail without adding much value. But maybe the value hasn't been added yet, and you've got plans to add it.

If I were trying to fix the core problem of batching playback of deletes (and the tweaked version I made of your very first patch worked fine w/o any kind of batching, I thought), when the user holds down the delete key, I would probably do something simple like set a timer on the first delete. If I get a second delete before the timer has fired, I just reset the timer to fire again in 500ms. Otherwise, I'd clear the timer and fire off an offline playback. In effect, once the user stops deleting messages within half a second of each other, then we would playback all the deletes at once. 

As you point out, it's awkward because the offline playback happens with the source folder, and the destination folder is the *this* in ::CopyMessages. And there are ownership questions about the nsImapOfflineSync object. Since we've added a wrapper around nsImapOfflineSync (nsPlaybackRequest), it seems like nsPlaybackRequest could own an nsRefPtr to nsImapOfflineSync...though the nsImapOfflineSync object owns a reference to the url listener that's passed into it, which is the nsPlaybackRequest, so that would be a circular ref that would need to be cleared whenever the playback request is finished. A very simple approach would be to add an nsPlaybackRequest reference to nsImapMailFolder, and store that in the src imap mail folder in CopyMessages, if there isn't one already, and fire its timer. When the timer fires, clear the reference, and execute the playback. If there was already a playback request, you don't have to do anything other than record the offline op. It will get run when the timer fires... Since this all happens on the UI thread, there aren't race conditions.


>No, it means Translation Unit. This is a similar technique to what 'static'

What's a Translation Unit? An instance of the program? It's not going to be a comment that's very meaningful for many people.
>We do it the other way around - unless we have a pretty good idea that the
>code's going to be used, we don't add it to the cvs repository.  Otherwise, we
>have to spend more time taking it out later.
Fair enough.

>I can't tell how much of the reason you made this new class is because stuff is
>going to get added to it eventually,
Well, one thing I couldn't communicate very well that I was thinking of this bug and bug 436615 as the milestones of the same effort, Better Faster IMAP, and was planning an incremental implementation. Actually that's why I named them the way it is. It made sense to me especially from testing point of view to develop the _base_ of a manager class that includes a queue with this patch, and eventually convert it into the Priority Queue component (ref wiki page given at Description) in the following phases. Manager class is implemented that in mind from the start.

>the automatic downloading process seems
>fairly separate from pseudo-offline playback.
Agreed, in term of being a product feature, they are. But I thought that they  overlap each other in term of implementation.

>and how much of it is because you weren't comfortable with your 
>knowledge of how the existing code works and felt it was easier to control >things with a new layer of code...
Right. Additionally I would add the reasons IMAP layer being error prone and difficult to maintain.

>I'm just not convinced that this extra class is needed at all for
>pseudo-offline playback.
Going to submit a new patch in line with the first approach.

>Right. Additionally I would add the reasons IMAP layer being error prone and
>difficult to maintain.
This is very true, but you're stuck relying on that layer to work correctly to report errors to you :-(

I'm not convinced we need a complicated imap operation url queuing mechanism for the offline download stuff - I think we can sort the list of messages we intend to download based on our criteria, and then just chain the running of the urls to fetch the messages. The list of messages could be thought of as a queue, and maybe there would be things that would cause us to re-arrange that list, but my preference is definitely to try the simple approach first and see where that breaks down...

I'm really looking forward to running with your patch - it makes deleting messages so much nicer. Thanks for being patient with me.
>I'm not convinced we need a complicated imap operation url queuing mechanism
>for the offline download stuff - I think we can sort the list of messages we
>intend to download based on our criteria, and then just chain the running of
>the urls to fetch the messages. 
Agreed, if we don't have to do prioritization between playback requests and offline download requests, we don't need an extra mechanism.

>Thanks for being patient with me.
This is the way how this process works I guess.
Attached file Patch rev 6: Minimal changes (obsolete) — 
The one with nsITimer in nsImapMailFolder.
Attachment #324331 - Attachment is obsolete: true
Attachment #324331 - Flags: review?(bienvenu)
Attachment #324881 - Flags: review?(bienvenu)
Comment on attachment 324881 [details]
Patch rev 6: Minimal changes

I'll look at this more closely tomorrow - but it looks like you're creating the timer in the constructor - would it be possible to create it on demand, when the folder first has a message moved/copied to it? Most folders won't need one. If I have 100 imap folders, there's no need for 100 timers.
Comment on attachment 324881 [details]
Patch rev 6: Minimal changes

this looks good in general...

maybe the pendingPlaybackRequest can own the timer - then the imap folder doesn't have to have a member var for it.

could this be a const instead?
+ enum { PLAYBACK_TIMER_INTERVAL_IN_MS = 500 }; 

Should we delete m_PendingPlaybackReq in the destructor, if somehow the user manages to shutdown with a pending request timer going?

if the playback request owns the timer, then it could clear the timer in it's destructor, in order to make certain that we don't have a timer going after the owning folder goes away. Since you're not holding a reference to the nsImapMailFolder in the playback request, you need to be vigilant about making sure you don't live past the life of the folder.
>maybe the pendingPlaybackRequest can own the timer - then the imap folder
>doesn't have to have a member var for it.
Although playback request cannot be more than one any given time, we delete it when the playback is completed. It means that we have to create a new timer for each new request. Per our discussion, I will keep the timer in folder, but move the initialization from ctor to do it lazily on demand.
Attached file Patch rev 7 (obsolete) — 
Changed according to comments.

On a side note; one advantage of having playback manager was to create only one timer for all playback requests on all imap folders on all imap accounts. It was optimal solution for resource usage. If 1-o-1 timer nsImapMailFolder association (via single request or directly) becomes a real concern, previous approach can be reconsidered.
Attachment #324881 - Attachment is obsolete: true
Attachment #324881 - Flags: review?(bienvenu)
Comment on attachment 324984 [details]
Patch rev 7

looks good - a couple minor things:

+  // cleanup any pending request
+  if (m_PendingPlaybackReq)
+  {
+    delete m_PendingPlaybackReq;
+    m_PendingPlaybackReq = nsnull;
+  }
 }

this can just be 

delete m_pendingRequest;

delete checks for null, and you don't have to null out the pointer since we're in the destructor already.

If you're comfortable that this works, maybe this could be removed?

+#if DEBUG_ebirol
+  nsCString destFolderName;
+  nsCString srcFolderName;
+  this->GetURI(destFolderName);
+  srcFolder->GetURI(srcFolderName);
+  printf("*** Copy request for destination folder %s and source folder %s\n", destFolderName.get(), srcFolderName.get());
+#endif

If this fails:

+    if (!srcImapFolder->m_PlaybackTimer) 
+      srcImapFolder->CreatePlaybackTimer();
+    
+    if (srcImapFolder->m_PlaybackTimer) 


maybe we want to return NS_ERROR_OUT_OF_MEMORY

similarly,
+        srcImapFolder->m_PendingPlaybackReq = new nsPlaybackRequest(srcImapFolder, msgWindow);

if that fails, again, we're out of memory.
Attached file Patch rev 8 — 
Changed according to comments.
Attachment #324984 - Attachment is obsolete: true
Attachment #325012 - Flags: review?(bienvenu)
Comment on attachment 325012 [details]
Patch rev 8

this looks good - I'll try it out.

I might change m_PendingPlaybackReq to m_pendingPlaybackReq just to be consistent with the naming convention we use for member vars...and m_playbackTimer, but I can do that before I check in, assuming everything works fine.
Attachment #325012 - Flags: review?(bienvenu) → review+
this is what I'm going to land - I just tweaked the member var names, and ran some tests. It seems to work well; thx again for being patient, Emre.
Hi everyone! I was looking for something just like this. Great you already worked on this. Will this be included in any nightly builds? Or could someone provide me a build for Windows? I'd love to test it! :) Helge
I checked this in for Emre yesterday so it should be in today's build.
Status: NEW → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9
I have it installed. Just great! Thank you very much. IMAP feels much, much better now!
(In reply to comment #53)
> I have it installed. Just great! Thank you very much. IMAP feels much, much
> better now!
> 

If you find any bugs, please be sure to report them. :)
Blocks: 331211
I will. I noticed no bugs so far.

The IMAP user experience is now way faster, but I am still faster at deleting my mail. When I want to delete multiple single mails, after deleting the first one, I still have to wait a little until it vanishes before I can delete the next. And I delete a lot of Emails. Could you do something about that, too? Would be great!!



Oh my! Please ignore my last comment. How embarrassing: I just startet an old build... the new build with your patch installed is so blazing fast!!! Sorry.
Depends on: 444417
Product: Core → MailNews Core
This should be mentioned in the Performance improvements section of the release notes.
Keywords: relnote
No longer blocks: 331211
Keywords: relnote
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: