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

Remove num-rational #1986

Merged
merged 1 commit into from
Aug 26, 2023
Merged

Remove num-rational #1986

merged 1 commit into from
Aug 26, 2023

Conversation

CryZe
Copy link
Contributor

@CryZe CryZe commented Aug 26, 2023

This removes num-rational as a dependency. As it turns out, num-rational, based on which features are active, can very easily end up on the critical path during compilation, meaning that it is the crate that determines how long it takes to compile the image crate. This is because cargo can parallelize compilation of many crates, but crates with a long dependency chains (rather than wide), are the ones that in the end determine the compilation speed. This is one of the learnings of the serde drama, where it has been noticed that allowing serde and serde_derive to compile in parallel reduces its long dependency chain, resulting in faster compile times. This introduces the same kind of optimization here, where for all the following features, num-rational turns out to be the longest dependency chain:

png,jpeg,gif,bmp,ico,pnm,tga,tiff,webp,hdr,dxt,dds,farbfeld,qoi

The critical path is the following:

autocfgnum-traits compile build.rs → num-traits run build.rs → num-traitsnum-integernum-rationalimage

As it turns out, the only thing really used in num-rational is Ratio, which can be easily replicated.

This cuts out both num-integer and num-rational cutting compile times by around 0.5 seconds on a debug build.

image

@CryZe
Copy link
Contributor Author

CryZe commented Aug 26, 2023

The only behavioral difference is that the ratios don't get simplified anymore. I'm not sure if that's important.

@CryZe CryZe force-pushed the remove-num-rational branch 3 times, most recently from b44b52b to ef05ab2 Compare August 26, 2023 17:30
This removes `num-rational` as a dependency. As it turns out,
`num-rational`, based on which features are active, can very easily end
up on the critical path during compilation, meaning that it is the crate
that determines how long it takes to compile the `image` crate. This is
because `cargo` can parallelize compilation of many crates, but crates
with a long dependency chains (rather than wide), are the ones that in
the end determine the compilation speed. This is one of the learnings of
the `serde` drama, where it has been noticed that allowing `serde` and
`serde_derive` to compile in parallel reduces its long dependency chain,
resulting in faster compile times. This introduces the same kind of
optimization here, where for all the following features, `num-rational`
turns out to be the longest dependency chain:

`png,jpeg,gif,bmp,ico,pnm,tga,tiff,webp,hdr,dxt,dds,farbfeld,qoi`

The critical path is the following:

`autocfg` → `num-traits` compile build.rs → `num-traits` run build.rs →
`num-traits` → `num-integer` → `num-rational` → `image`

As it turns out, the only thing really used in `num-rational` is
`Ratio`, which can be easily replicated.

This cuts out both `num-integer` and `num-rational` cutting compile
times by around 0.5 seconds on a debug build.
@fintelia fintelia merged commit da79fe9 into image-rs:master Aug 26, 2023
35 checks passed
@CryZe CryZe deleted the remove-num-rational branch August 26, 2023 21:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants