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

Replace lazy_static with phf and once_cell #143

Closed
wants to merge 1 commit into from

Conversation

GnomedDev
Copy link
Contributor

@GnomedDev GnomedDev commented Jul 18, 2022

  • Reduces macro magic in the case of replacing with once_cell::sync::Lazy and removes a lot of lines due to no indentation required.
  • Improves runtime performance in the case of replacing lazy_static'd HashMap with phf::Map by creating the maps at compile time, avoiding even one allocation at runtime

@pdeljanov
Copy link
Owner

Hi @GnomedDev,

It looks like you put quite a lot of work into this. Unfortunately, I see some problems that give me pause:

  1. Regarding the replacement of lazy_static with once_cell. According to the once_cell docs, lazy_static has an advantage in terms of supporting #![no_std]. I think we do want to eventually support no_std and we having a tracking ticket here #![no_std] support #88. Are there any other benefits besides code brevity that outweigh this drawback?
  2. In terms phf there are two issues I see:
    • First, phf requires a MSRV of 1.60, while we support down to 1.53. Rust 1.60 is rather new. I was thinking of moving to 1.56 next. Either way, increasing the MSRV would be done for v0.6.
    • Second, since the hash tables are being generated at compile time, how does that affect binary size vs. the existing solution? We've already had a few issues raised regarding binary size and I'd prefer trading a few CPU cycles for a smaller binary.

@GnomedDev
Copy link
Contributor Author

Hey, sorry for completely missing your response.

  1. once_cell is now a part of the Rust standard library, so the library should probably move to that if a breaking change is in order
  2. I personally think if this change would be made, it move to a simple match statement instead of dragging in a dependency, if possible.

Since this has been left stale and is now conflicting so much, I will be closing this PR, feel free to use the ideas or code for a future PR.

@GnomedDev GnomedDev closed this Nov 7, 2023
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