On Fri, Feb 17, 2012 at 7:34 AM, Paolo Bonzini <pbonz...@redhat.com> wrote: > On 02/17/2012 02:18 PM, Alon Levy wrote: >>> > - guaranteed bitrot in the poll() path :) >>> > >> Yeah, I guess you are right. I'm worried since I don't know the >> difference in performance between epoll and poll > > You can assume it to be zero unless you have a lot of descriptors (50 is > probably not enough) and a lot of wakeups (if you wake up once every 10 > ms, even spending 0.1 ms to process the 50 descriptors would be 1%). > > If it turns out to be a problem, you can always "git revert", I doubt > this part of the code has churn.
There were a lot of responses here, so I'll try to cover all the points raised. * epoll vs. poll difference: if you throw multiclient mode aside for a minute, we have a max of *3* file descriptors involved here. select()/poll()/epoll()/kqueue performance differences will be non-existent at this level, epoll/kqueue were designed to handle the C10K problem, and as Paolo stated, there will be no difference when we're talking single or double-digit amounts of file descriptors. * epoll with poll fallback: bitrot absolutely stinks, and there is zero need for it here, given the above information about performance. ifdef is even worse as it becomes trivial to introduce a compile failure on another platform when you change something the preprocessor omits from the to-be-compiled processed code. * ifdef/vtable: the overhead of a vtable would probably match any performance difference of switching to poll from epoll. * libevent: this was my original idea actually, but when I realized the trivial number of FDs we were dealing with, I realize it made no sense. Additionally, our event "loop" here is anything but standard, with all the refcounting business we do post-poll() call. libevent basically expects to keep running the loop forever, so running all that code once per loop involves a rather large refactor which is much more dangerous than the relatively straightforward changes I had to make here. Let's say I got something to compile but it was a total mess going to libevent, and I felt way less sure about that then this patch. Alon- if multiple client support is experimental, you'll be fine with dropping MAX_EVENT_SOURCES back to 10, which would still support up to 4 total clients connecting for now; anyone that wanted more can tweak this as necessary, and when the support becomes non-experimental, this can turn into a resizable array rather than a hardcoded length. Thanks! -Dan _______________________________________________ Spice-devel mailing list Spice-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/spice-devel
