Skip to content
New issue

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

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add faster versions of `equals` and `is_equal` #107

Open
wants to merge 1 commit into
base: master
from

Conversation

@asppsa
Copy link

asppsa commented Aug 31, 2019

Hi there,

I'm just getting my feet wet with Rust/Rutie, so its entirely possible that this patch is nonsensical. Also, I'm not sure if I have put things in the right place, etc. Happy to make any changes you consider necessary!

First, because Ruby < 2.5 has a different implementation of rb_eql, I've added a bunch of cfg directives to build.rs, so that functions can be defined conditional on the Ruby version. This means that the faster is_eql function is only used on Ruby 2.5+. Presumably I should document these somewhere, but I haven't done this. The remaining changes are:

  • Object::equals is now a wrapper around the rb_equal C function;
  • Object::is_eql is now a wrapper around the rb_eql C function on Ruby 2.5+;
  • Object::is_equal simply performs an == comparison on the underlying values, which is the same behaviour as the rb_obj_equal C function.
@asppsa

This comment has been minimized.

Copy link
Author

asppsa commented Aug 31, 2019

OK, so in Ruby 2.4, rb_eql returns an int instead of a VALUE ...

@asppsa asppsa force-pushed the asppsa:faster_equality branch from 9934920 to 0332d8f Aug 31, 2019
build.rs Outdated
@@ -40,6 +40,24 @@ fn macos_static_ruby_dep() {
#[cfg(not(target_os = "windows"))]
fn windows_static_ruby_dep() {}

fn rustc_cfg_ruby_version() {
let program_version = rbconfig("RUBY_PROGRAM_VERSION");
let v: Vec<u32> = program_version.split('.').map(|s| s.parse::<u32>().unwrap()).collect();

This comment has been minimized.

Copy link
@calavera

calavera Sep 24, 2019

Contributor

This is great. I just want to leave a comment, because RUBY_PROGRAM_VERSION works for preview releases too, so this code won't break in cases where you're using a pre-release or an RC:

➜  ~ chruby 2.7.0-preview1
➜  ~ irb
irb(main):001:0> RbConfig::CONFIG["RUBY_PROGRAM_VERSION"]
=> "2.7.0"
irb(main):002:0> 
@calavera

This comment has been minimized.

Copy link
Contributor

calavera commented Sep 24, 2019

This is a very good point. Hopefully it will get into master soon 🙏

@danielpclark

This comment has been minimized.

Copy link
Owner

danielpclark commented Oct 16, 2019

I'm hesitant to allow Ruby versioning for different code paths. Once you travel down this road it grows into an unmaintainable beast. I have some code commented out in Rutie waiting for "End of Life" (our own EOL timing, not Ruby's… see below) of older Ruby versions before I can use Ruby's frozen feature in certain places.

Once Ruby 2.7 comes out we can stop officially supporting 2.4 as we only support the 3 most recent versions of Ruby. Then you won't need this Ruby version check. So you just need to wait until Christmas for adding this new implementation… or work with a fork of this project until then.

@asppsa

This comment has been minimized.

Copy link
Author

asppsa commented Oct 16, 2019

Sounds reasonable! I'll rework this PR some time in the lead-up to xmas

- `Object::equals` is now a wrapper around the `rb_equal` C function;
- `Object::is_equal` simply performs an `==` comparison on the
  underlying values, which is the same behaviour as the `rb_obj_equal` C
  function.
@asppsa asppsa force-pushed the asppsa:faster_equality branch from 0332d8f to 48bfe11 Nov 11, 2019
@asppsa asppsa changed the title Add faster versions of `equals`, `is_eql` and `is_equal` Add faster versions of `equals` and `is_equal` Nov 11, 2019
@asppsa

This comment has been minimized.

Copy link
Author

asppsa commented Nov 11, 2019

I've dropped the is_eql implementation from this PR, so it now works without the versioning. I'll try to remember to come back & open a follow-up PR once 2.7 gets released.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.