[please do not merge] Rewrite one method in rust #787
Conversation
With much thanks to this blog post: https://blog.codeminer42.com/integrating-ruby-with-rust-with-ffi/ In order to deploy this, we would first need to add cargo to the lib-jobs boxes (perhaps through apt). Currently, the code is in a crate of type cdylib, which does not support doctests! 😢 I wonder if we could use the cdylib as a compatibility layer, and then use a lib crate (with doctests) to actually provide the functionality. We should also make sure we are running the following regularly, perhaps as part of CI? * `cargo clippy` * `cargo test` * `cargo fmt` * `cargo doc` I am also confused as to whether or not this contains a memory leak. FFI is returning a two-element array, with our string and then a pointer. But if I try to free the pointer afterwards (which I could be doing wrong), I get an error that it hasn't been allocated? Very mysterious (and ironic that we have to worry about it when using memory safe languages like Ruby and Rust).
| @@ -5,6 +5,11 @@ module LcCallSlips | |||
| # Marc::DataField for the keywords that a selector | |||
| # is interested in. | |||
| class KeywordField | |||
| extend FFI::Library | |||
| c_lib_extension = /darwin/.match?(RUBY_PLATFORM) ? 'dylib' : 'so' | |||
There was a problem hiding this comment.
Annoyingly, Mac and Linux use different file extension for dynamic libraries.
| extend FFI::Library | ||
| c_lib_extension = /darwin/.match?(RUBY_PLATFORM) ? 'dylib' : 'so' | ||
| ffi_lib Rails.root.join('lib_jobs_rs', 'target', 'release', "liblib_jobs.#{c_lib_extension}") | ||
| attach_function :normalize_keyword, [:string], :strptr |
There was a problem hiding this comment.
normalize_keyword is the name of our rust function, and we specify that it takes a string and returns a string pointer.
| @@ -41,7 +46,7 @@ def word_is_keyword?(word) | |||
| end | |||
|
|
|||
| def normalize(word) | |||
| word.sub(/[[:punct:]]?$/, '') | |||
| normalize_keyword(word)[0] | |||
There was a problem hiding this comment.
Instead of the original Ruby implementation, use the normalize_keyword function that we attached earlier.
| @@ -0,0 +1,4 @@ | |||
| # frozen_string_literal: true | |||
There was a problem hiding this comment.
Compile the rust code each time we start rails or run the tests
| /// there are some safety considerations described in | ||
| /// <https://doc.rust-lang.org/std/ffi/struct.CStr.html#method.from_ptr> | ||
| #[no_mangle] | ||
| pub unsafe extern "C" fn normalize_keyword(raw_string_ptr: *const i8) -> *const i8 { |
There was a problem hiding this comment.
This function is mainly glue code: taking the pointer that ruby provided, calling the logic of our function, and then returning a new pointer to the corrected string.
There was a problem hiding this comment.
I don't think we need the unsafe keyword in the function signature since the only unsafe call is wrapped in an unsafe block. We don't need to use an unsafe block in the test if this function isn't marked as unsafe.
| .into_raw() | ||
| } | ||
|
|
||
| fn normalize_string(original_string: &str) -> &str { |
There was a problem hiding this comment.
The actual Rust implementation of the original Ruby method
Co-authored-by: Ryan Laddusaw <[email protected]>
With much thanks to this blog post:
https://blog.codeminer42.com/integrating-ruby-with-rust-with-ffi/
In order to deploy this, we would first need to add cargo to the lib-jobs boxes (perhaps through apt).
Currently, the code is in a crate of type cdylib, which does not support doctests! 😢 I wonder if we could use the cdylib as a compatibility layer, and then use a lib crate (with doctests) to actually provide the functionality, with doctests.
We should also make sure we are running the following regularly, perhaps as part of CI?
cargo clippycargo testcargo fmtcargo docThe original blog post said that we need to
freethe memory after we are done, using the pointer we get from calling the ffi function in the second element in the array. But when I try that, I get "calling free on non allocated pointer", and the ffi gem documentation says that the garbage collecter will do this for us... I hope this is true, it would be ironic to add memory leaks when using two memory safe languages 😬