rust-rrule icon indicating copy to clipboard operation
rust-rrule copied to clipboard

replace regex with custom parsers

Open oll3 opened this issue 1 year ago • 6 comments

This PR removes both the regex and lazy_static as direct dependencies. This mainly to decrease the binary size.

In release mode, with stripped binary (x86_64), a simple RRuleSet parse example is decreased with ~1.1 MiB compared to when build before this PR. I haven't measured how it compares performance wise.

oll3 avatar May 20 '24 13:05 oll3

Hey! Can you share the exact commands you ran to get that information? 🙏 I am a bit surprised the binary size would reduce as regex would still be a transitive dependency of rrule even if it was not directly specified in Cargo.toml.

fmeringdal avatar Jun 02 '24 14:06 fmeringdal

Hey! Can you share the exact commands you ran to get that information? 🙏 I am a bit surprised the binary size would reduce as regex would still be a transitive dependency of rrule even if it was not directly specified in Cargo.toml.

Hi and thanks for responding!

Yes, I think you are partly right... Though, if I read the dependency tree correct, regex is only an (indirect) build-time dependency of chrono-tz so it's never included in built binary.

My sample program looks like below (Cargo.toml+main.rs):

[package]
name = "rrules-test"
edition = "2021"
[profile.release]
strip = true
[dependencies]
rrule = "0.12"
fn main() {
    let exp = std::env::args().nth(1).unwrap();
    let rrule: rrule::RRuleSet = exp.parse().unwrap();
    println!("{:#?}", rrule);
}

When I build this program (in release mode) prior to this PR the size of the binary is 3979720 bytes. After the PR it's 2812328 bytes.

oll3 avatar Jun 02 '24 21:06 oll3

Fixed a lint:

51 |         let (time, zulu_timezone_set) = if len >= 15 && len <= 16 && &val[8..9] == "T" {
   |                                            ^^^^^^^^^^^^^^^^^^^^^^ help: use: `(15..=16).contains(&len)`

oll3 avatar Jun 03 '24 06:06 oll3

regex is only an (indirect) build-time dependency of chrono-tz so it's never included in built binary.

I missed that, makes sense.

I have to admit that I am a bit skeptical of merging this as the regex parsing has worked well and IIRC was copied from some other much more used rrule implementation in another language. The size reduction is quite impressive though!

Do you have a use-case where the size reduction would be beneficial? I can imagine that this crate is already too big to be used in any resource constrained devices

fmeringdal avatar Jun 03 '24 22:06 fmeringdal

Do you have a use-case where the size reduction would be beneficial? I can imagine that this crate is already too big to be used in any resource constrained devices

It's for use on a resource constrained device yes. :) Not that 1 MiB is impossible to fit but it's quite a waste of both precious IoT bandwidth and disk space. But I do understand that tested and true is valuable so sure understand your side. Would adding more tests make any difference?

oll3 avatar Jun 03 '24 22:06 oll3

Rebased on main and added test and fix for the error @WhyNotHugo pointed out.

oll3 avatar Jun 10 '24 08:06 oll3