Re: [libmicrohttpd] Expose the fdset events

[Christian Grothoff]

Dear Simon,

A few comments right now (as this seems to be a bit of a preliminary
patch for a larger idea):

1) If you have/had/write a kqueue event manager implementation and we
   can compile that instead of epoll on FreeBSD systems, we should ship
   that with MHD. I wonder if we should use the same numeric value for
   kqueue_freebsd_only that we currently have for epoll_linux_only...

2) Your API includes a bunch of 'typedef'. I *hate* typedef's (for
   anything but function types), as they obscure the types used and
   introduce additional exported symbols.  So please don't use those.

3) Using ":1" bitfield-style annotations is also (typically) overkill
   and may force the compiler to generate useless bit-shifting code.
   It's generally fine to just do 'int' instead. In terms of space,
   it is more important to consider the order of the fields in a
   struct, so your 'struct EventManager' should have the 'int's next
   to each other (preferably after the pointers) to avoid alignment
   holes.  Ditto for the other structs (if you have multiple flags,
   an int8 instead of an 'int' can be fine to reduce memory consumption)

4) timeout_update should also have an EventManager as 1st argument,
   ditto for timeout_free

5) I'm worried about the need for the event manager to allocate some
   'struct' (MHD_Watch); that suggests a possible additional failure
   mode ('malloc' failed), and may create unnecessary overheads.
   We can probably fix this by having a non-opaque

struct MHD_Watch
{
   int fd;
   void *em_ctx;
   MHD_WatchEvent events,
   MHD_WatchCallback callback,
   struct MHD_Connection *mhd_data;
};

   allocated as part of 'MHD_Connection', allowing the EventManager
   to store its context in the 'em_ctx' *if necessary* but avoiding
   the extra allocation.  Some of the MHD_Watch fields already exist
   in MHD_Connection, so we'd essentially just move those into that
   internal struct which we expose to the EM.  That way, watch_new
   can just return 'void' and we eliminate the failure mode.
   (we might call the APIs then watch_start/watch_stop instead).

Finally, it seems you wrote the EM-API only for 'external' event loops.
If you added an additional function or two for 'blocking' and/or
running, we should be able to use the same for the internal event loops
and the thread pools, turning the existing select/poll/epoll logic into
instantiations of that EM-API, which would then make this endeavor much
more worthwhile IMO.


Happy hacking!

Christian


On 06/05/2015 01:57 AM, Simon Newton wrote:
> I have a change that seems to work, at least I've tested with http &
> https using an external kqueue/kevent event manager.
> 
> Christian: do you mind looking over
> https://github.com/nomis52/libmicrohttpd/pull/1/files which has the
> API & an example program. If you're happy with the approach I'll clean
> up the rest of the code so you can review it.
> 
> Simon
> 
> 
> On Thu, Jun 4, 2015 at 8:05 AM, Simon Newton <address@hidden> wrote:
>> On Wed, Jun 3, 2015 at 1:08 AM, Christian Grothoff <address@hidden> wrote:
>>> Well, the easiest way you can do it now is just ask MHD to run in
>>> 'EPOLL' mode and grab the epoll() FD. Then you can select/poll/epoll on
>>> the MHD epoll() FD.  You won't get detailed information about add/remove
>>> events from MHD, but you'll then only have to deal with one FD for all
>>> of MHD in your event set ever, and your overall big-O complexity for all
>>> operations should also be perfect.
>>
>> That only works for platforms with epoll() support.
>>
>> I'll have a go at building a generic event management API today.
>>
>> Simon
>>
>>> My 2 cents
>>>
>>> Christian
>>>
>>> On 06/03/2015 04:33 AM, Simon Newton wrote:
>>>> Digging up a thread from the past...
>>>>
>>>> Is the codebase in a better shape to make this change now? We've run
>>>> into this again now that we've switched to using kevent rather than
>>>> select.
>>>>
>>>>
>>>> If you're looking for existing APIs to model this from,  Avahi has a
>>>> watch API [1] which provides fd notifications, and that has worked
>>>> well for us.
>>>>
>>>> [1] http://avahi.org/download/doxygen/struct_avahi_poll.html
>>>>
>>>>
>>>> Simon
>>>>
>

signature.asc
Description: OpenPGP digital signature