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

Speed up --all-features build by 33%~55% by removing cfg #24

Merged
merged 61 commits into from
Oct 27, 2023

Conversation

workingjubilee
Copy link
Contributor

@workingjubilee workingjubilee commented Oct 12, 2023

Currently, this library enables a cfg(feature = "{name}") for every single icon.

Unfortunately, feature resolution the way Cargo does it is not, and cannot, be "fast" beyond a certain point: it is at minimum a time cost that is linear with the number of features. At its worst, the package resolution problem is often equated with bool-SAT. In addition, this adds several thousands of lines of code to each crate which must be lexed, parsed, and so on. As I recall, adding a feature is also going to invalidate the build cache for a crate.

My personal machine is very fast, but this still makes the builds notably faster.

cargo... main remove-cfg-gen
check --all-features ~14.5s ~11.5s
build --all-features ~16.5s ~12.5s

I believe almost everything is in order, here, but I found difficulty testing the result meaningfully. One of the libraries did not finish downloading so I backed out the generated diffs involving it.

@workingjubilee
Copy link
Contributor Author

This has different effects on the zero-feature build case for this repo.

However, your dependents are going to be enabling features, or else this crate will be roughly equivalent to another Rust file that is also very fast to compile:

When they add a feature, they will discard the incremental build cache again on the next compiler run. In many cases, people will have a compiling daemon build their crate periodically, which, unless they type very fast, will result in discarding and rebuilding this crate once for each feature they add, if memory serves. After doing this even 100 times, I expect, this will make node_modules seem lightweight.

Meanwhile, each individual crate itself does not seem to suffer a notable impact from being built without cfg. This also affects the download of the crate and its index (which rust-lang/crates.io#7269 mentions as containing all the features), which are also factors, especially in first-time addition of this crate or other fresh-slate builds.

@carloskiki
Copy link
Owner

Thank you for reaching out!

The first intentions with protecting every single icon behind a feature is to reduce wasm bundle sizes. I believe web-sys is also feature gating their structs for the same reason but I may be wrong 😅. After benchmarking on my machine, it seems that not using feature flags per icon and importing only 1 icon crate (i.e., icondata_fa) bloats the bundle by 1.5MB!!

I'm afraid we will have to find an alternative solution to resolve this issue. I am fully aware of the stress this creates on Cargo and crates.io, and I am willing to look into solutions if you can think of any.

@workingjubilee
Copy link
Contributor Author

workingjubilee commented Oct 12, 2023

The web-sys crate is a direly undermaintained crate hacked together several years ago, when wasm tooling was much worse, yet expected to get better relatively quickly to the point of outmoding it in a few years, and then given up for other maintainers, who have since changed it further. There is no one who can meaningfully answer for its design choices anymore, so I will neither excuse them nor accuse them. I will say, however, that I think that any design choice that seems reasonable can become unreasonable if you add a few orders of magnitude to it.

From what I understand of how the compiler, linker, and other tools do dead code elimination, I believe that by creating a single type which references every single icon, the per-crate enum which then becomes an IconData, your crate's style virtually guarantees that any use of the enum with outside data that could make it take on any value would make dead code elimination almost impossible. That is assuming, of course, that the compilation tools adequately perform dead code elimination. It is plausible they do not.

If you wanted to assure that only used code is injected into the bundle, why not design your crate as a build tool, fundamentally? Via build.rs or otherwise. You're already asking the programmer to list the icons they want, after all. This won't improve the initial build times, of course, but it will shift the burden to more appropriate locations and put more control in the hands of the actual user.

@workingjubilee
Copy link
Contributor Author

workingjubilee commented Oct 12, 2023

I believe there are three good solutions:

  • Work to significantly improve dead code elimination in general in the Rust/wasm ecosystem. May be slightly blue-sky.
  • Shift the build towards the final package, by asking programmers to provide a file that enumerates the list of icons they want, stowed in a variable Cargo can track so that it can rerun-if-changed in the build.rs, that then builds the data in, so that it is more a "just-in-time" phenomenon.
  • Using statics instead of consts (so that the data is guaranteed to be in a single place), possibly combined with #[link_section] and possibly emitting metadata that makes it easier to track what gets actually used and put it into its own sections, and then DCE sections of statics quasi-manually using wasm-strip or the like.

@carloskiki
Copy link
Owner

I believe there are three good solutions:

  • Work to significantly improve dead code elimination in general in the Rust/wasm ecosystem. May be slightly blue-sky.

I believe I am highly underqualified for this job, and I will leave it to someone who is more familiar with rust and compiler architectures.

  • Shift the build towards the final package, by asking programmers to provide a file that enumerates the list of icons they want, stowed in a variable Cargo can track so that it can rerun-if-changed in the build.rs, that then builds the data in, so that it is more a "just-in-time" phenomenon.

This idea is for me the most compelling, and something I will try to achieve for the next release.

  • Using statics instead of consts (so that the data is guaranteed to be in a single place), possibly combined with #[link_section] and possibly emitting metadata that makes it easier to track what gets actually used and put it into its own sections, and then DCE sections of statics quasi-manually using wasm-strip or the like.

Similar to the first option, I do not know enough about Rust's ABI and wasm to undertake such a possibility.

I had another idea which I wanted to have your opinion on since you most certainly have more experience than me: How about a proc-macro that has the whole icon data-table and that inlines the data at compile time?

Thank you, your help means a lot!

@workingjubilee
Copy link
Contributor Author

workingjubilee commented Oct 16, 2023

I had another idea which I wanted to have your opinion on since you most certainly have more experience than me: How about a proc-macro that has the whole icon data-table and that inlines the data at compile time?

That seems reasonable. It's "morally equivalent" to the build.rs version, I think, since it's basically a big fat build step either way, but it may have different implications regarding deduplication across multiple dependents. I think your dominant use-case is likely to be just 1 or 2 crates using this library, so you should probably focus on whatever you feel has better user ergonomics.

@workingjubilee
Copy link
Contributor Author

Also, I believe either this PR or a similar solution of your own design is going to block the next release, due to this:

They aren't designing for more than 32K features maximum, which means that even if they granted you an exemption, this package would be dead in the water with only one or two more icon libraries added to it assuming you remained with its current design. Judging by their comments on the Rust Zulip, it seems to be because they basically only think this strategy is okay when it's about a thousand features (and even those require exemptions: the default limit is 300). I believe you want something that is capable of having 3 more icon libraries added to it, so that isn't going to work out for you.

You may want to consider accepting this PR so that you have the option of shipping a new release immediately, despite the binary size regression, and then doing the redesigns on top of it.

@carloskiki carloskiki merged commit 7f93f1a into carloskiki:main Oct 27, 2023
12 checks passed
@workingjubilee workingjubilee deleted the remove-cfg-gen branch February 3, 2024 03:22
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.

None yet

2 participants