From: Matthias A. <mat...@gm...> - 2010-04-26 16:18:58
|
Rainer Weikusat wrote on 2010-04-26: > This looks very suspiciously like my first attempt at making a 'quick' > improvement to this code. It will yield about half of the benefit of > the patch I sent (for 'our' application), because this method cannot > be used for the fastuidl path in pop3_is_old (as you wrote, I profiled > this before making the more complicated change). It is also (sorry for > being so blunt) not very well done. Because of the > > /* do it nonrecursively so the list is in the right order */ > for (end = idl; *end; end = &(*end)->next) > continue; > > in save_str_quick, the value passed to the routine should be the > address of the pointer which needs to be changed when appending to the > list, eg, assuming the 'savep' change: Yeah, we can save that last single iteration from end to &end->next and go all the way, and we can use a proper initializer. Would make only a minor difference though as long as we're still doing linear searches. > a better implementation would be > > svp = &ctl->oldsaved; > > [...] > > last_nr = try_nr; > /* save it */ > sv = save_str(svp, id, UID_UNSEEN); > sv->val.status.num = try_nr; > svp = &sv->next; > --------------------------- Thanks. > which has the nice bonus property that it doesn't have the conditional > which goes one way during the first iteration and the other way during > all that follow inside the loop (the idea isn't mine, I originally > learnt about it because of some years-old USENET posting of a guy > whose name I've unfortunately forgotten). I am also rather concerned > about use of bandwidth than speed. Presently, I am dealing with 76 > POP3 accounts and about a meg of stored UIDs I'd need to download > every five minutes in order to determine that presently, nothing needs > to be downloaded (this refers to fastuidl in general, assuming I > understood the principle correctly without really analyzing the code). Makes sense, although I'm wondering if it really makes that much of a difference for fastuidl. fastuidl appears to be opportunistically harvesting message numbers, I wonder if that's of any use. I think Sunil wrote that fastUIDL code, I'm Cc:ing him. > So, thank you very much for your effort, but I'll stick with the > version in the private fork I am anyway maintaining because of the > additional features (like 'object-oriented/ vtabled sinks') whose > usefulness would be very limited outside this particular > application and/or whose implementation is rather 'commercial' (eg > #ifdefing away the concurrency control code) than aesthetically/ > technically pleasing. Not sure I get your point. I've always wanted to abstract the sink code the way the fetch protocols are abstracted, and that was also one thing I'd tried to squeeze from the 2008 Google Summer of Code that was supposed to provide MAPI (but apparently isn't fit for integration, and apparently stalled). > The savedend-change was just something I > considered to be more generally useful, hence I 'backported' it to > 6.3.16 from my HEAD and sent it to the list. Much appreciated. > BTW, I had a look at the UIDL patch and whoever wrote that should > probably spend some time reading RFC4549 (Synchronization Operations > for Disconnected IMAP4 Clients) which is what I am going to add to the > imap-code because using the \Seen flag as 'message is old' indicator > [reportedly] interferes with the gmail web interface. I'd appreciate if such code could be made public. The server-side \Seen tracking is something I have wanted fetchmail to get rid of for long. The long-winded way with "smtp_someop() { if (I'm surprisingly using mda) mda_someop() else if (using bsmtp) else if (using lmtp) }" is garbage, inconcise, inefficient -- and it appears from your description you've fixed just that already. > NB: Nothing of this text is meant as an insult even if it may sound > like one. No offense taken. I can usually tell the difference between criticing my work objectively, subjectively, and ad-hominem attacks. :) -- Matthias Andree |