Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

unix, windows: pass structs by address, not value #684

Closed
bnoordhuis opened this Issue Jan 15, 2013 · 14 comments

Comments

Projects
None yet
7 participants
Contributor

bnoordhuis commented Jan 15, 2013

Functions like uv_tcp_bind() and uv_tcp_connect() take a struct sockaddr_in as a value, make them use pointers instead.

Why?

  1. Dealing with struct values is a pain for bindings authors.
  2. Lets us slim down the API. Example: uv_tcp_bind(struct sockaddr_in) and uv_tcp_bind(struct sockaddr_in6) become uv_tcp_bind(struct sockaddr*) (that's what happens internally anyway).

Target v0.12.

/cc @piscisaureus

@bnoordhuis bnoordhuis referenced this issue in orthecreedence/cl-async Jan 15, 2013

Closed

libuv #32

fillest commented Jan 15, 2013

Does it apply to callbacks like uv_read_cb? For example with LuaJIT FFI structs that are passed by value to callback are disallowed so one has to make complicated wrappers.

Contributor

bnoordhuis commented Jan 15, 2013

You mean functions/callbacks that take uv_buf_t structs? I'm undecided, depends on how hard it makes life for people.

From an ABI perspective, there's no difference between void f(uv_buf_t buf) and void f(void* base, size_t size), both push two arguments on the stack or pass them in registers, depending on the calling convention. (On x86/x86_64. I'm reasonably sure other architectures do the same thing, it'd be illogic not to.)

So it depends on how difficult the current function prototype is to work with and how inflexible FFI implementations are.

fillest commented Jan 15, 2013

You mean functions/callbacks that take uv_buf_t structs?

Yes, for example:
typedef void (*uv_read_cb)(uv_stream_t* stream, ssize_t nread, uv_buf_t buf)

Well, at the moment i have to deal with it this way:
I declare typedef void (*net_read_cb_t) (uv_stream_t* tcp_handle, ssize_t nread, uv_buf_t *buf);
Make a global empty net_read_cb_t net_read_cb; (global because i will need it in C funcion later)
In FFI'ed script i assign actual callback to this global net_read_cb
The intermediate callback to pass is

void net_on_read (uv_stream_t* tcp_handle, ssize_t nread, uv_buf_t buf) {
    net_read_cb(tcp_handle, nread, &buf);
}

And the final invocation: C.uv_read_start req.handle, C.net_on_alloc, C.net_on_read
I haven't thought about overhead or thread-safety yet - making it to just work was too much for that time :]
So... :D

Contributor

bnoordhuis commented Jan 15, 2013

Right. What you can do as a workaround is to declare your function like this:

void net_read_cb(uv_stream_t*, ssize_t, void* ptr, size_t len) {
  // ...
}

And call uv_start_read() like this:

uv_read_start(stream, alloc_cb, (uv_read_cb) net_read_cb);

Kind of hacky but it works because of the aforementioned calling convention rules. Admittedly, I don't know how complicated that is with Lua/LuaJIT's FFI (if it's possible at all.)

Contributor

bnoordhuis commented Jan 15, 2013

I guess the above is a good argument for replacing uv_buf_t with explicit ptr/len parameters...

Member

piscisaureus commented Jan 16, 2013

@bnoordhuis I agree in general but not for uv_buf_t.

Contributor

txdv commented Jan 18, 2013

While doing what this issue describes (txdv/libuv@165d729) I found some inconsistent error code behavior which I want to get rid of in a separate commit (#689)

fillest commented Jan 18, 2013

@piscisaureus

but not for uv_buf_t

Why?

@fillest FYI, I talked to the luajit author about this exact issue many months ago. His recommendation was to not use ffi with anything callback based at all. The callbacks make the ffi considerably slower, even if you work around the by-val struct references. My plan which I never found time to do, was to wrap the libuv API in a more poll style API where I repeadly call into libuv asking for the next event and it blocks till there is an event and then returns the event as a struct pointer. According to Mike, that should be lot faster than using the callback interface with ffi. But like I said, I never found time to write such a wrapper (in C) to libuv.

fillest commented Jan 21, 2013

@creationix Yeah I'm aware of it, I've read it many times. The funny thing is: there are no public benchmarks or profiling results which clearly shows it (correct me if I'm wrong), and it seems everybody just repeat this mantra :) It can be slow in absolute numbers but negligible in relative to other operations (so it may be critical for one application and irrelevant for another).
For example I've made a load testing tool (not yet publicly released) with lua (c api, jit was used later, without using of ffi) and http_parser (callbacks yeah), and IIRC profiling showed most time was being spent in string processing, ungzipping and other stuff. Not exacly the same case but you get the idea.
Now I'm writing a multiplayer game server, it's a prototype (though luajit/ffi/libuv were chosen for some good reasons) so performance of ffi'ing callbacks doesn't worry me at the moment. And what does, is to ship fast and to reinvent as little as possible while having good enough performance. Libuv has saved me a lot of time generally, but it would be great for people doing similar things if they could just literally pass a callback to the function (if uv_buf_t would be passed by reference to callback) without creating twisted crutches.
Polling-style wrapper would be great of course anyway but sadly I don't have time yet too. If it will become a considerable performance problem (well I doubt it, I have some quite heavy stuff like physics processing), I will have to make it..

Sh4rK commented Jan 24, 2013

@creationix and @fillest I also wanted to do a poll style wrapper for libuv for some time, and already started it, so if you can wait a little, maybe you don't have to write your own, if you find mine good enough. And of course, once I publish it (or even before that), suggestions are welcome :)

Sh4rK commented Feb 3, 2013

What's the status of this feature? Is it getting in libuv in the near future?

Contributor

bnoordhuis commented Feb 3, 2013

What's the status of this feature? Is it getting in libuv in the near future?

Depends on your definition of 'near future'. After node.js v0.10 at the earliest.

@bnoordhuis bnoordhuis closed this in 3fb6612 Sep 2, 2013

@bnoordhuis bnoordhuis added a commit that referenced this issue Sep 2, 2013

@bnoordhuis bnoordhuis include: uv_read{2}_cb now takes const uv_buf_t*
Passing or returning structs as values makes life hard for people that
work with libuv through a foreign function interface. Switch to a
pointer-based approach.

Fixes #684.
b7d027c

@bnoordhuis bnoordhuis added a commit that referenced this issue Sep 2, 2013

@bnoordhuis bnoordhuis include: uv_ip[46]_addr now takes sockaddr_in*
Passing or returning structs as values makes life hard for people that
work with libuv through a foreign function interface. Switch to a
pointer-based approach.

Fixes #684.
8184076

@bnoordhuis bnoordhuis added a commit that referenced this issue Sep 2, 2013

@bnoordhuis bnoordhuis include: uv_tcp_bind{6} now takes sockaddr_in*
Passing or returning structs as values makes life hard for people that
work with libuv through a foreign function interface. Switch to a
pointer-based approach.

Fixes #684.
daa229a

@bnoordhuis bnoordhuis added a commit that referenced this issue Sep 2, 2013

@bnoordhuis bnoordhuis include: uv_tcp_connect{6} now takes sockaddr_in*
Passing or returning structs as values makes life hard for people that
work with libuv through a foreign function interface. Switch to a
pointer-based approach.

Fixes #684.
255671d

@bnoordhuis bnoordhuis added a commit that referenced this issue Sep 2, 2013

@bnoordhuis bnoordhuis include: uv_udp_recv_cb now takes const uv_buf_t*
Passing or returning structs as values makes life hard for people that
work with libuv through a foreign function interface. Switch to a
pointer-based approach.

Fixes #684.
0f7b296

@bnoordhuis bnoordhuis added a commit that referenced this issue Sep 2, 2013

@bnoordhuis bnoordhuis include: uv_udp_bind{6} now takes sockaddr_in*
Passing or returning structs as values makes life hard for people that
work with libuv through a foreign function interface. Switch to a
pointer-based approach.

Fixes #684.
525dbb5

@bnoordhuis bnoordhuis added a commit that referenced this issue Sep 2, 2013

@bnoordhuis bnoordhuis include: uv_udp_send{6} now takes sockaddr_in*
Passing or returning structs as values makes life hard for people that
work with libuv through a foreign function interface. Switch to a
pointer-based approach.

Fixes #684.
263da51

@bnoordhuis bnoordhuis added a commit that referenced this issue Sep 2, 2013

@bnoordhuis bnoordhuis include: uv_spawn takes const uv_process_options_t*
Passing or returning structs as values makes life hard for people that
work with libuv through a foreign function interface. Switch to a
pointer-based approach.

Fixes #684.
8c6ea10

Excellent! Time to give wrapping libuv in lisp another shot. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment