Rust python module with helpers for meta-memcache #1
Conversation
bisho
force-pushed
the
initial_import
branch
7 times, most recently
from
5fd65e7 to
1b416d8
Compare
There was a problem hiding this comment.
Took a look. Looks good at first glance, couple of questions:
- Should
.gitignoreinclude everything from the template? Might be better to cut it to what's needed - We should remove
lib.rs.oldI think, no need for it since the refactor to separate files - You can have tests in a separate file if you wish to keep the files smaller, following the pattern in khepri auto load shedder and metrics exporter, where the tests are in
mod testsintests.rsandmod tests;is added tolib.rsto make sure the module is imported.
TIL: maturin
What template? This was the default created for the maturin project (I did not manually tunned it), happy to change it if there is a better template.
Ops
I read the convention in rust is unit tests in the same file, integration tests in separate module. But no strong opinion on this. |
## Motivation / Description Implementing this in rust is simple and benchmarking the rust cost gives 20-50 ns runtime. Unfortunately a lot of time is spent in the bridge between python and rust, so a call to rust code is not cheap, yet this implementation speeds up some meta-memcache code significantly.
src/impl_build_cmd.rs
Outdated
| } | ||
|
|
||
| #[cfg(test)] | ||
| mod tests { |
There was a problem hiding this comment.
Is this the way to make tests in Rust? I thought you should have them in another file or something
There was a problem hiding this comment.
Apparently, for unit tests... but it also bugs me... I'll move them.
There was a problem hiding this comment.
Yes it's idiomatic Rust to do tests in the same file, but given you can have multiple files in the same directory with mod BLAH and the compiler will treat it all as being in the same module (there's a separate file scope).. we can abandon that convention and keep files nice and small for easier reviews.
|
|
||
| pub fn impl_parse_header( | ||
| data: &[u8], | ||
| start: usize, |
There was a problem hiding this comment.
nit: Same here with usize. I assume the data.len is not going to be greater than 65k?
There was a problem hiding this comment.
You can arguably configure a receive buffer of more than 64KB depending on your usage and avg value len...
There was a problem hiding this comment.
It's looking great! Super excited to see this out in the wild.
By the way, if you want code coverage, you should be able to adapt cov.sh from the envoy modules.
Giving you an approval so you aren't blocked, but of course @ethervoid should approve too given the open comments
Good point, default template is OK I guess. Just that it had a ton of extraneous stuff. It's always subjective whether to include the everything or be minimal and strip it down to just what's needed, either way is fine so if you prefer to stick with the template |
bisho
force-pushed
the
initial_import
branch
from
082611e to
3c64ccf
Compare
bisho
force-pushed
the
initial_import
branch
from
3c64ccf to
5d05e92
Compare
Motivation / Description
Implementing this in rust is simple and benchmarking
the rust cost gives 20-50 ns runtime. Unfortunately
a lot of time is spent in the bridge between python
and rust, so a call to rust code is not cheap, yet
this implementation speeds up some meta-memcache
code significantly.