Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

feature request: kqueue support #2

Closed
cactus opened this Issue · 23 comments
@cactus

feature request for kqueue support.

$ ./configure
....
configure: error: required sys/epoll.h header file is missing

It would be nice to be able to use twemproxy/nutcracker while doing development on osx, as well as possible deployment onto FreeBSD. Would it be possible to add kqueue support?

@manjuraj
Collaborator

I would be willing to take patches for kqueue support in twemproxy

@Ayutthaya

As a school project, I've made a patch to support kqueue, epoll and event ports, using the libevent API. There are also a few additional changes to make it work on my mac os x. (the patch is at github.com/ayutthaya/twemproxy ). I'm ready to improve my work if necessary, so don't hesitate to give feedback. Regards.

@manjuraj
Collaborator

Thanks for the patch using libevent API.

I am willing to accept patches if they don't use libevent but directly call bsd's kqueue API. All you might have to do is to implement abstraction in the event interfaces, which would be in nc_event.[ch]. Let if know if you have any questions.

@Ayutthaya

I changed the patch. Now it uses kqueue API directly (but doesn't support event ports anymore). It needed changes mainly to nc_event.[ch] and nc_stats.[ch], since stats aggregation was also based on epoll. The patch is still at github.com/ayutthaya/twemproxy . one question: what is the reason for not using libevent API ? Thanks.

@manjuraj
Collaborator

the reason I don't use libevent api is because I wanted to build nutcracker without any dependency on 3rd party libraries. furthermore, I use ET semantics, which I believe is not available in 1.4 version of libevent.

could i get a pull request of your changes?

I also looked at your changes. I think you might want to do few cleanups before submitting a pull request. Few suggestions:
a) abstract out the event interface and wire to the underlying event call using a function pointer
b) maybe create event/ directory that contains event/nc_epoll.c and event/nc_kqueue.c files.
c) follow style conventions outlined in: https://github.com/twitter/twemproxy/blob/master/notes/c-styleguide.txt

@Ayutthaya

I've tried to comply with the suggestions above as much as possible. In particular, I've abstracted out the event interface and used a function pointer to make all #ifdef HAVE_EPOLL/KQUEUE disappear in all files except nc_event.h nc_epoll.c and nc_kqueue.c. Don't hesitate if you think it needs further changes before doing a pull request.
I also have a question about twemproxy: what is the number of sockets from which point one should use twemproxy / the performance of the cache server starts degrading seriously due to per-connection overhead? 65k? 200k? If you could simply give me a lead, it would help me a lot for my project. Thanks !

@antirez

Such a support would be great for the project because today most developers have osx computers, even if I doubt twemproxy is going to be deployed on osx if not in very rare cases, the ability to try it on osx is a good get developers exposed feature.

@srned

@manjuraj I got a chance to work on this and the patch is at https://github.com/srned/twemproxy Let me know your thoughts and I can submit a pull request.

@Ayutthaya

as far as I'm concerned this is a good job srned ! couldn't finish mine, though I appreciated hacking on this very well-made open source code. the way you reorganized core_loop and core_core among other pieces looks familiar to me ;-) Hope your branch will be merged ! :-)

@charsyam

@srned good job!.

@Ayutthaya

@srned it seems you could finish what I started, well done

@manjuraj
Collaborator

@srned thanks! I am little bit overloaded right now. But, I promise I will take a look at your branch as soon as possible.

@srned

@Ayutthaya I am glad you liked it :-)
@manjuraj Thanks!

@kevwil

At the risk of showing my naivety with C code, I would like to understand why @manjuraj has taken this stance. Please forgive me if this is a silly question.

I don't understand the logic of asking contributors to implement event abstractions but not being willing to depend on existing event abstractions. Existing abstractions like libev and libuv are hardened and proven in real-world use, where a one-off abstraction implementation is neither, at least for a while. I don't think it's a performance issue, because accepting hand-coded abstractions from contributors may not yield any faster performance than existing abstraction libraries ... likely the performance might be a bit worse until the code is hardened. Abstractions like libev, or libuv even more so, would broaden the potential developer audience quite a bit. So ... why not just use libuv or libev? Aren't these hand-coded abstractions re-inventing the wheel?

@rottenbytes

@manjuraj any update on this one ?

@rrueth

@manjuraj Any updates on getting twemproxy to compile on OS X?

@manjuraj
Collaborator

I have pushed a branch -- twemproxy_bsd https://github.com/twitter/twemproxy/tree/twemproxy_bsd that contains kqueue support by @ferenyx #57

Please use this. I believe it is stable, but I need more confirmation from folks who use it. Please use it and report back if things are stable.

If the branch -- twemproxy_bsd has no issues for the next 30 days, I will merge it to master

@eranb

Thanks :+1:

@manjuraj
Collaborator

The twemproxy_bsd branch has been stable over last one month; I have merged @ferenyx patch - #57 to master. Thanks @ferenyx, and @Ayutthaya

I will bump the version # and release a new distributable soon

@manjuraj manjuraj closed this
@eranb

@manjuraj when do you plan to release a new distribution tarball ?

@manjuraj
Collaborator

Definiltey before the end of this month; I need to do some cleanups on head before realizing a tar ball

@magicgirl44

checking for sys/epoll.h... no
configure: error: required sys/epoll.h header file is missing

I still can not install twemproxy on OS X, has the project supported kqueue ?

@rraptorr

kqueue support is present in master branch, try that

@idning idning referenced this issue from a commit in idning/twemproxy
@idning idning nutcracker core on heavy multi-del
today we got a core of twemproxy::

    $ gdb -c core.14420 ./bin/nutcracker

    (gdb) bt
    #0  0x000000302af2e2ed in raise () from /lib64/tls/libc.so.6
    #1  0x000000302af2fa3e in abort () from /lib64/tls/libc.so.6
    #2  0x0000000000419c82 in nc_assert (cond=0x444dc0 "!TAILQ_EMPTY(&send_msgq) && nsend != 0", file=0x444aa8 "nc_message.c", line=745, panic=1) at nc_util.c:308
    #3  0x000000000040d0d6 in msg_send_chain (ctx=0x553090, conn=0x55b380, msg=0x0) at nc_message.c:745
    #4  0x000000000040d568 in msg_send (ctx=0x553090, conn=0x55b380) at nc_message.c:820
    #5  0x00000000004059af in core_send (ctx=0x553090, conn=0x55b380) at nc_core.c:173
    #6  0x0000000000405ffe in core_core (arg=0x55b380, events=65280) at nc_core.c:301
    #7  0x0000000000429297 in event_wait (evb=0x5652e0, timeout=389) at nc_epoll.c:269
    #8  0x000000000040606f in core_loop (ctx=0x553090) at nc_core.c:316
    #9  0x000000000041b109 in nc_run (nci=0x7fbfffea80) at nc.c:530
    #10 0x000000000041b20d in main (argc=14, argv=0x7fbfffecc8) at nc.c:579
    (gdb) f 3
    #3  0x000000000040d0d6 in msg_send_chain (ctx=0x553090, conn=0x55b380, msg=0x0) at nc_message.c:745
    745         ASSERT(!TAILQ_EMPTY(&send_msgq) && nsend != 0);
    (gdb) l
    740             if (msg == NULL) {
    741                 break;
    742             }
    743         }
    744
    745         ASSERT(!TAILQ_EMPTY(&send_msgq) && nsend != 0);
    746
    747         conn->smsg = NULL;
    748
    749         n = conn_sendv(conn, &sendv, nsend);

it is caused by this ``ASSERT`` at nc_message.c:745,

``conn_send`` send no more than ``NC_IOV_MAX(128)`` pieces in ``msg_send_chain``,

if the first fragment of MULTI-DEL response is send on last batch. and this is the last msg in send queue, the next call of ``msg_send_chain`` will got ``nsend == 0``::

following case show such a case:
1. mget on ``126`` keys
2. a mutli-del cmd

::

    def test_multi_delete_20140525():
        conn = redis.Redis('127.0.0.5', 4100)
        cnt = 126
        keys = ['key-%s'%i for i in range(cnt)]
        pipe = conn.pipeline(transaction=False)
        pipe.mget(keys)
        pipe.delete(*keys)
        print pipe.execute()

see: https://github.com/idning/test-twemproxy/blob/master/test_redis/test_del.py#L56-L63

more detail: http://idning.github.io/twemproxy-core-20140523.html
d5ec284
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.