Use rb_io_wait function and cache io instance.

[ioquatix]

https://github.com/github/activerecord-trilogy-adapter/issues/28

[matthewd]

Thanks for following up on this @ioquatix!

[ioquatix]

@matthewd can you please approve and run the workflows.

[ioquatix]

There is one more piece we should consider exposing, which is the "resolve DNS" to use the fiber scheduler hook.

[ioquatix]

So, writing this PR has made me realise we need a set of shims for the blocking C APIs, e.g. open, addrinfo, etc. We have the code, it's just not exposed. I supposed we can do that for Ruby 3.3.

We should also extend the test suite to run within a fiber scheduler (probably Async).

[matthewd]

@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?

[ioquatix]

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.

[kirs]

👋 just wanted to say that I appreciate this WIP PR by Samuel and I'd really really love to see this happen in the upstream, and to be able to use trilogy with fiber scheduler for IO-heavy workloads.

[casperisfine]

I'd really really love to see this happen in the upstream, and to be able to use trilogy with fiber scheduler for IO-heavy workloads.

So my knowledge of the fiber scheduler is limited, but AFAIK you can already use it today? My understanding is that this PR make it a bit more efficient, but rb_wait_for_single_fd is already enough to be usable.

As for it happening, there's not much work needed, all the blockers are listed in review comments.

[ioquatix]

I've rebased and updated the PR.

rb_wait_for_single_fd is okay but it's a slow path. rb_io_wait is the fast path.

@matthewd can you please run CI.

[ioquatix]

I recall why I gave up on this PR for a while, we were missing rb_io_open_descriptor which is needed to create a "temporary non-owning FMODE_EXTERNAL IO instance". Since that's now available in recent CRuby, it is possible to implement it correctly.

[ioquatix]

I believe all the feedback has been addressed, so this is good to go.

[eregon]

What's the performance gain with this?
Is it worth the duplication of _cb_ruby_wait and additional complexity?

[ioquatix]

What's the performance gain with this?

It should be slightly more efficient, but I imagine there isn't much performance difference (except for GC overhead due to the temporary IO objects created).

Is it worth the duplication of _cb_ruby_wait and additional complexity?

I would say yes. rb_wait_for_single_fd creates a temporary IO instance for each call, which creates extra (pointless) garbage. It's better for Ruby extensions that do IO to use a proper IO object IMHO. I would like to deprecate rb_wait_for_single_fd. It's messy/difficult for some platforms to implement, e.g. JRuby.

[ioquatix]

I think now that rb_trilogy_wait_protected was extracted out, maybe we can simplify this PR a little to address the complexity @eregon mentioned.