oxidize-rb / rb-sys

Easily build Ruby native extensions in Rust
https://oxidize-rb.github.io/rb-sys/
Apache License 2.0
230 stars 35 forks source link

Add initial implementation of a stable API #229

Closed ianks closed 1 year ago

ianks commented 1 year ago

As part of #227, this PR implements a the basis for a stable ABI for Ruby on Rust. For companies that run ruby-head in production, ABI instability is a serious priority, since without having stability it's hard (if not impossible) to test out certain changes to Ruby.

This can be incredibly disruptive to both Ruby core, as well as Rust gem authors. The goal here is to always Just Work™️ with ruby-head, but without sacrificing performance on stable Ruby.

The core idea consists of two main parts.

1) C Macro + Inline Function Support

In Rust, we don't get access to the suite of Ruby C macros or inline functions in libruby. So we have to either:

This PR makes it so rb-sys does both, so we get the best of both worlds. On known stable versions of Ruby, we use the hand-written Rust implementations of the Ruby macros. On ruby-head, we use the compiled C glue code.

2) Make internal Ruby data structures opaque

This PR also adds a new stable-abi feature for rb-sys, which will make the internal layout of various Ruby structs opaque (i.e. RString and RArray). With this feature enabled, it will become impossible for upstream crates to have ABI instability when using the opaque struct.

For now, this feature is off by default (with some deprecation warnings) to allow for adoption (with. In v1.0, it will be enabled by default.

matsadler commented 1 year ago

Sorry if this is a bit scattered. I've a bunch of thoughts about this but not the time to write them up as one coherent message.

The core idea, provide Rust implementations of C macros/inline functions, and link to the C stubs of the same, totally seems like a reasonable one. However.


My understanding is that ABI (Application Binary Interface) is the interface to the binary code of a precompiled library. So with a stable ABI you can dynamically link to a library and not worry about the exact version. For example Ruby 3.2.0 and Ruby 3.2.2 have the same ABI, an extension gem compiled against 3.2.0 will continue to work with 3.2.2 without having to recompile it.

API (Application Programming Interface) is the interface as described by the source code.

You can have a change that doesn't break the ABI while it does break the API, for example:

struct user {
    char *name;
    // unused, must be filled with 0
    void *reserved[7];
}

and

struct user {
    char *name;
    char *address;
    // unused, must be filled with 0
    void *reserved[6];
}

are ABI compatible as they are the same size, and the name field doesn't move. Something that was compiled against the first version doesn't realise the address field exists, but it still works. However recompiling code written for the first will break.

From my understanding (which is not that of an expert, so plenty of opportunity for me to be wrong) I believe the change that kicked off this issue was ABI stable (as noted in the commit, the location of len in memory doesn't change), but a breaking change in the API.

Even with the docs that have been added to Ruby's API it's still so hard to understand the intention behind a lot of stuff, but I'm working under the assumption that Ruby's RString struct and it's fields are part of the public API. It's possible that's something that developers of Ruby would prefer they can take back, but as far as I can tell they've not decided to do that yet.

My understanding is the internal in ruby/include/ruby/internal/core/rstring.h is saying that the name/location of the header file isn't intended to be public, that instead the public interface to to include ruby/ruby.h/ But that includes .../rstring.h, so you get all the same stuff.

There are other structs, RStruct for example, that are opaque to the public API, so I don't think it's a case C not providing the tools to make RString opaque.

I do worry that this is a whole lot of work to get nowhere. What happens when there's a Ruby API change in head that say just removes a function that was unarguably public in the past? Doesn't that just break in exactly the same way and this does nothing to address that?


On a completely different topic, Cargo features should always be additive, as when a library is compiled Cargo sums up all the features of all the crates in the current project and compiles once with all the features enabled. If a feature takes things away, then one dependency can enable it and break another dependency that was relying on the feature not being enabled.


It seems to me that low-level bindings should hew as closely as possible to the original C API, hiding details because it's easy to use in a way that could break seems like something for a higher level library.


Personally I would prefer the fallback to C macro/inline stubs to be off by default so people have to explicitly opt-in to the potentially significantly slower C stubs, rather than things get mysteriously slower when they upgrade Ruby.


This does make rb-sys a bigger dependency for mr (magnus is pretty low on direct dependencies, and I like it like that), and move some stuff out of my direct control (so say there's a bug in the code in question, right now I can just fix it and do a release of magnus, that changes if it moves to rb-sys). Which makes me a little reticent, but that's probably just me being too emotionally attached to the thing I built.


I'd be much happier with:

ianks commented 1 year ago

Even with the docs that have been added to Ruby's API it's still so hard to understand the intention behind a lot of stuff, but I'm working under the assumption that Ruby's RString struct and it's fields are part of the public API. It's possible that's something that developers of Ruby would prefer they can take back, but as far as I can tell they've not decided to do that yet.

My understanding is that these fields really should be private, but due to limitations with C it's non-trivial to accomplish, and doing so would break a lot of existing extensions. I could be wrong though, will follow up.

In general, the large public surface area of libruby has been problematical for the Ruby team. Exploratory breaking changes are hard to test without breaking existing C extensions. Rust can help a lot here by being more intentional about what's marked as pub.

swap ABI to API (or better educate me on which is which)

Happy to do this. I think that naming makes more sense.

C macro/inline stubs behind a feature (so dependency on C compiler etc can be disabled and no surprise slowness)

This one is tough. We really do need to have things "just work" with Ruby head which we cannot know ahead of time. To give a concrete reason of why this is important, we have multiple gems unable to ship gem upgrades on the core monolith and SFR. This caused disruption for both our Ruby core team and Rust gem authors.

The middle-ground I decided on was to not actually attempt to compile anything unless we do in fact need to compiled macros (i.e. we are on ruby-head). So nothing will be compiled unless absolutely necessary. Unfortunately, this means we have an unconditional dependency on the cc crate but that seems like a fair trade off.

As for the surprise slowness when upgrading Ruby:

  1. I've yet to measure how much of a performance impact that will actually cause. I'll get some numbers together to better understand.
  2. It's already impossible to predict if an implementation of a macro will be correct for a future version version of Ruby, and I think correctness is paramount here.
  3. rb-sys will guarantee that a stable-api implementation of released version (i.e. 3.3) will exist at the time of release.
  4. Item 3 requires that users actually upgrade rb-sys to get those performance benefits. We can explore some ways to nudge upstream dependencies to cargo update -p rb-sys.

don't make RString/RArray structs opaque (or if you do, arrange it so they are opaque, but there's a default feature that gives you full access (and that feature won't be deprecated), that way people who want the compiler errors if they accidentally use it can turn off that feature, but you don't fall into the subtractive features trap)

That makes sense. Currently, I have the logic so that stable-abi (off by default) feature will opaque-ify the structs. I'll invert that logic.

As another alternative, the rb_sys::unstable mod could be unconditionally exposed which would always expose these internal fields. Thoughts @matsadler ?

ianks commented 1 year ago

Ok so @k0kubun graciously took some time to help answer the question about public fields in RString, etc:

I think we want C extensions to access the fields efficiently by using a C macro rather than making a regular C function call since that’d be relatively more frequent than method calls on those objects.

and to make C macros work, their definition should be usable as is; so the fields must be exposed.

To hide the details we want to hide, we would lose performance in C extensions.

Since it’s not mentioned in the docs, you can quote me on fact that C macros need to see the definition of those structs, which we would ideally like to hide but don’t for performance reasons.

ianks commented 1 year ago

Ok so (I believe) I’ve implemented some of the suggestions regarding renaming and opaqueness @matsadler.

For the opaqueness thing, I think for 0.9 I’ll have to remove the unstable-api feature altogether so extensions on Magnus v0.5 can make use of these changes once it’s implemented upstream. This means the default exported bindings will always contain the internal deprecated struct fields.

The opaque structs will be accessible still at rb_sys::bindings::stable. For 1.0, I’d be delighted to have the the rb_sys::bindings::stable be exported from the root, but make it so bindings::unstable will always be accessible if needed (for break-glass-in-case-of-emergency situations).

As for feature-flagged compilation, I described the issues in https://github.com/oxidize-rb/rb-sys/pull/229#issuecomment-1598964944. If you have some time, let me know what you think about it, and potential solutions that you see that I may have overlooked.

Want to get this into a place that everyone is happy with, so please don’t hesitate to leave more comments/concerns here (however big or small they may be). ❤️

matsadler commented 1 year ago

Ok so @k0kubun graciously took some time to help answer the question about public fields in RString, etc:

I think we want C extensions to access the fields efficiently by using a C macro rather than making a regular C function call since that’d be relatively more frequent than method calls on those objects. and to make C macros work, their definition should be usable as is; so the fields must be exposed. To hide the details we want to hide, we would lose performance in C extensions. Since it’s not mentioned in the docs, you can quote me on fact that C macros need to see the definition of those structs, which we would ideally like to hide but don’t for performance reasons.

Ah, I see, RStruct can be private because the accessors to internal state (e.g. RSTRUCT_GET) are inline functions, whereas RString still has some macros as part of its api?

C macro/inline stubs behind a feature (so dependency on C compiler etc can be disabled and no surprise slowness)

This one is tough. We really do need to have things "just work" with Ruby head which we cannot know ahead of time. To give a concrete reason of why this is important, we have multiple gems unable to ship gem upgrades on the core monolith and SFR. This caused disruption for both our Ruby core team and Rust gem authors.

From my perspective testing against Ruby head is very very uncommon. It's just waisting dev/CI time downloading and compiling that cc crate for every user of rb-sys outside of Shopify. Plus I can imagine the amount of effort that someone could go to in diagnosing why their app got slower when they upgraded Ruby, only to find a dependency of a dependency is written in Rust and needs to be updated. If the C fallback was behind a feature it seems like it'd be really easy to add rb-sys = { version = "*", features = "c-fallback" } to your gems' Cargo.toml, which would then enable it transitively for all dependencies.

ianks commented 1 year ago

From my perspective testing against Ruby head is very very uncommon.

For applications, I think you are right. It's rare. However, applications typically use published versions of Rust gems which are typically precompiled binaries anyway. So there's not compilation overhead there.

For Rust gems, not testing native extensions against ruby-head is an anti-pattern, IMO. In the end it makes it much harder to test and measure development features in Ruby head which the entire Ruby community benefit from.

It's just waisting dev/CI time downloading and compiling that cc crate for every user of rb-sys outside of Shopify.

I wanted to measure the impact of the CC crate on compilation times to see how big of a concern this might be. In the oxi-test repo, here are the numbers I get:

Screenshot 2023-06-23 at 9 12 08 AM

Overall its a 700ms-ish, of which only a small portion is blocking. From eyeballing it, the cost ends up being on the order of 100-200ms.

On top of that, cc often ends up being a transitive dependency in crates, which means there is no compilation extra cost.

Plus I can imagine the amount of effort that someone could go to in diagnosing why their app got slower when they upgraded Ruby, only to find a dependency of a dependency is written in Rust and needs to be updated.

Going to get some more precise numbers for this, but anecdotally our largest internal gems there was no noticeable difference in performance (despite fairly heavy usage of as_str and such)

The alternative would be to use an outdated Rust implementation of the Ruby macro which could go one of three ways:

  1. Work perfectly
  2. Fail to compile due to breaking changes
  3. Be subtly incorrect and induce safety issues

So overall I think the slightly slower implementation is favorable in most cases.

ianks commented 1 year ago

That being said, it should be pretty easy to nuke the cc dep and just use std::process if that would help ease concerns?

ianks commented 1 year ago

Got a couple of benchmarks together for rstring_len and rstring_ptr here. All of the below were run from my macbook m1 with Ruby 3.2.

str from rstring (unchecked)

This benchmark creates a Rust str from an RString - with no utf8 checking

violin

str from rstring (checked)

Same as above, but with UTF8 checking.

violin (1)

In these two examples, Rust implementation wins out with larger strings (1024 bytes) when strictly measuring the throughput of the functions. However, once you throw UTF8 validation in the mix, the performance difference becomes insignificant. I imagine this is mostly due to the extremely low overhead extern C calls to static functions, and the minimal benefit offered from inlining in the RString case since it's mostly a few flag checks and pointer bumps.#233

Going to measure some other things as well.

ianks commented 1 year ago

The cc dependency is now gone ✌🏻

ianks commented 1 year ago

Alright, so after thinking on what @matsadler said, I've decided to make the compiled C fallback optional (behind a stable-abi-compiled-fallback feature flag or --cfg=rb_sys_use_stable_api_compiled_fallback). This makes it easy to toggle on when required without risking unintended slowdowns, etc.

With that all said and done, I think this is close to being good to go. Just testing out a few more things on some gems in prod.

ianks commented 1 year ago

The windows failures are unrelated, and will be fixed in #221. Going to go ahead and merge this.

To be clear, this PR adds an opt-in stable-api with the ability to support ruby-head. It adds zero dependencie - in fact, it removes one! Typically, users should interact with the macros that are backed by this stable API (i.e. RSTRING_PTR, etc).

❤️