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 once_cell #418

Merged
merged 1 commit into from
Nov 6, 2023

Conversation

hinto-janai
Copy link
Contributor

rust-lang-nursery/lazy-static.rs#214

once_cell can be faster (10~20%) in contentious retrieval as well, test examples here https://github.com/matklad/once_cell/tree/master/examples - although you'll have to swap in lazy_static and test yourself.

Once std::sync::LazyLock is stabilized, Lazy can be renamed and once_cell can be removed altogether. Tracking issue.

@kayabaNerve
Copy link
Member

Thanks for the interest!

We currently have both lazy_static and once_cell in-tree due to ecosystem fragmentation. Accordingly, any edits here don't actually reduce the entire amount of dependencies in our dependency graph. Accordingly, this PR must be evaluated for what goal it works towards/immediate benefits.

We've prior replaced lazy_static with std options. Even though std's lazy isn't stabilized, its OnceLock is. It has a worse API, requiring a helper function be defined with it, yet it removes all non-std dependencies. While again, this doesn't apply to our entire tree (due to dependencies which haven't updated), it does at least move parts of our codebase to purging third party crates from their sub-trees for this purpose.

once_cell is another lazy_static. It may be a better lazy_static, yet that doesn't change it's another lazy_static. A 3rd party library for which 1st party alternatives now sort-of exist. While I do still lean towards merging this, as why not + slightly reduced compile times, I don't see such a clear-cut benefit compared to moving to OnceLock + helper function.

I'll merge once the CI passes 👍 Thanks for the interest, and the support. Let me know if you'd be interested in a follow up commit to replace once_cell with OnceLock.

@kayabaNerve kayabaNerve added the improvement This could be better label Nov 5, 2023
@hinto-janai
Copy link
Contributor Author

Let me know if you'd be interested in a follow up commit to replace once_cell with OnceLock

Sure - to be clear there are no instances of OnceLock in serai, do you mean you'll start using once_cell::Once from now on and those should be replaced in the future? If so, sure.

Although considering the dependency tree, lazy_static and once_cell will probably be used for the rest of time even if serai doesn't use them :)

@kayabaNerve
Copy link
Member

coins/ does use OnceLock, as re-exported from std by std-shims when building for std platforms.

I can dream regarding achieving a minimal dependency tree :P

lazy_static::lazy_static! {
static ref INIT_LOGGER: () = env_logger::init();
}
static INIT_LOGGER: once_cell::sync::Lazy<()> = once_cell::sync::Lazy::new(|| env_logger::init);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should actually be written as ::new(env_logger::init), not ::new(|| env_logger::init()). clippy will likely complain otherwise.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for all the pushes, I don't have librocksdb so can't test locally :)

The last push failed because clippy wanted the function item directly, it wanted ::new(env_logger::init) instead of ::new(|| env_logger::init()), I just forgot to get rid of the closure.

@kayabaNerve kayabaNerve merged commit bd3272a into serai-dex:develop Nov 6, 2023
14 checks passed
@kayabaNerve
Copy link
Member

Thanks again. I'll also look at removing lazy_static from our substrate dependencies, as that should remove most consumers of lazy_static in our tree...

@hinto-janai hinto-janai deleted the lazy branch May 9, 2024 21:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
improvement This could be better
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants