Use rb_io_wait function and cache io instance.
#47
Conversation
| { | ||
| struct trilogy_ctx *ctx = ptr; | ||
|
|
||
| if (RTEST(ctx->io)) |
There was a problem hiding this comment.
Mark moveable doesn't check if the field is NULL, IIRC without this it can crash.
There was a problem hiding this comment.
I was looking at https://github.com/ruby/ruby/blob/1fdaa0666086529b3aae2d509a2e71c4247c3a12/gc.c#L7108 and https://github.com/ruby/ruby/blob/1fdaa0666086529b3aae2d509a2e71c4247c3a12/gc.c#L10395 -- I'm not 100% sure it's part of their defined contract, but I imagine it must be true by ossification, even if not originally intended?
Intuitively I just prefer "this slot contains a a possible GC ref", rather than an [even trivial] encoding of "we might need to mark this". 🤷🏻♂️
There was a problem hiding this comment.
I agree it's possibly not the greatest interface.
| @@ -183,6 +205,7 @@ static double timeval_to_double(struct timeval tv) | |||
|
|
|||
| static int _cb_ruby_wait(trilogy_sock_t *sock, trilogy_wait_t wait) | |||
| { | |||
| struct trilogy_ctx *ctx = sock->user_data; | |||
There was a problem hiding this comment.
This almost feels like there's a danger of a possible lifetime issue here, but I think it's actually fine.
There was a problem hiding this comment.
I didn't look too closely at the lifetimes but I assume the socket should outlive the context?
|
|
||
| return 0; | ||
| VALUE result = rb_io_wait(ctx->io, RB_INT2NUM(wait_flag), rb_fiber_scheduler_make_timeout(timeout)); |
There was a problem hiding this comment.
I acknowledge it's what's already happening on sufficiently-new rubies (along with the IO construction), but rb_fiber_scheduler_make_timeout sounds like it might allocate. Is that true? Something we can/should avoid?
There was a problem hiding this comment.
There is a small chance I the future it might allocate but for the moment it doesn't, it just embeds a float into the VALUE. I hope at some point in the future to migrate to fixed point timeouts.
There was a problem hiding this comment.
Bonus question: is this going to affect our min ruby version (which I note we don't seem to currently declare)? I'm guessing this is moderately recent.
There was a problem hiding this comment.
No I'll add feature detection, this PR is just what I could manage in the 30 minutes I had before bed time.
There was a problem hiding this comment.
it just embeds a float into the VALUE
Flonums only work for a subset of all double, if it doesn't fit it will be an allocation.
There was a problem hiding this comment.
I think it also always allocate on 32bits?
|
@matthewd can you please approve and run the workflows. |
ioquatix
force-pushed
the
main
branch
2 times, most recently
from
0965d54
to
61269c1
Compare
|
There is one more piece we should consider exposing, which is the "resolve DNS" to use the fiber scheduler hook. |
|
So, writing this PR has made me realise we need a set of shims for the blocking C APIs, e.g. We should also extend the test suite to run within a fiber scheduler (probably Async). |
|
@ioquatix is this good to go? It seems fine to me, not sure if you've kept it in draft for a reason, or just not hit the button? |
|
It's probably as good as we can make it.. but I want to review it this weekend. This week has been pretty busy, have not had much time for open source stuff. |
|
|
||
| #ifdef TRILOGY_RB_IO_WAIT | ||
| if (ctx->io == Qnil) { | ||
| ctx->io = rb_io_fdopen(fd, O_RDWR, NULL); |
There was a problem hiding this comment.
trilogy_data_type is marked with RUBY_TYPED_WB_PROTECTED, so you have to trigger a write barrier here.
Problem is you don't have access to the VALUE here.
There was a problem hiding this comment.
Okay, so we should pass around the VALUE then?
There was a problem hiding this comment.
Either that, or create the IO eagerly in a place where you have access to it.
https://github.com/github/activerecord-trilogy-adapter/issues/28