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

Modernizing LazyStatic APIs #111

Open
matklad opened this Issue Aug 3, 2018 · 10 comments

Comments

Projects
None yet
4 participants
@matklad
Copy link

matklad commented Aug 3, 2018

Context: https://internals.rust-lang.org/t/pre-rfc-lazy-static-move-to-std/7993/36

Rust has progressed enough to make lazy static API less magical. It could look like this (example from once_cell):

static GLOBAL_DATA: Lazy<Mutex<HashMap<i32, String>>> = sync_lazy! {
    let mut m = HashMap::new();
    m.insert(13, "Spica".to_string());
    m.insert(74, "Hoyten".to_string());
    Mutex::new(m)
};

I am creating this issue to discuss what we can do with this exciting possibility :-)

Just to be clear, I am explicitly not suggesting that we should deprecate the current API and switch to the new shiny. There's a ton of code in the wild which uses lazy_static!, and that is great.

Nevertheless, I think the current API has some problems, and the possible new API has less of them! Specifically, the current lazy_static! macro is opaque: it has a somewhat unique syntax, which is Rustish, but is not exactly Rust, it creates a unique hidden type behind the scenes and the implementation is hard to follow. I think this are significant drawbacks, especially from the learnability perspective.

When a new rustecean asks "how can I have global data?", the typical answer is: "you need to lazily initialize data on the first access. This is what C++ and Java do at the language level, but Rust this is achieved via the lazy_static library". And then a rustecan goes to see how lazy_static is implemented, sees this and thinks "wow, this is almost as horrifying as STL implementations" (well, at least that was my reaction :D).

I'd want to argue that an explicit Lazy<T> would be clearer at the call site (no more magical unique types) and much easier to understand (you just Ctrl+B/F12/M-./gD and read the impl).

An interesting facet of the new API is that Lazy does not need to be static!

So, are folks excited about the possibility of getting rid of lazy_static! macro? I propose the following plan for this:

  • we extract sync::Lazy from once_cell and publish it as a separate rust-lang-nursery crate, sync_lazy, with the following API
pub struct Lazy<T, F = fn() -> T> { ... }

impl<T, F: FnOnce() -> T> {
  pub /*const*/ fn new(f: F) -> Lazy<T> { ... }
  pub fn force(this: &Lazy<T, F>) -> &T { ... }
}

impl <T, F: FnOnce() -> T> Deref for Lazy<T, F> {
  Target = T;
  fn deref(&self) -> &T { ... }
}

macro_rules! sync_lazy {
    ($($body:tt)*) => { ... }
}
  • sync_lazy crate uses parking_lot::Once. That way, we don't need to wait until non-static Once is stable, and we also give parking_lot some more testing, as was requested in rust-lang/rfcs#1632 (hence, cc @Amanieu)

  • In the lazy_static readme, we point that one might try sync_lazy instead.

@matklad

This comment has been minimized.

Copy link
Author

matklad commented Aug 3, 2018

Here's the proposes sync_lazy implementation: https://github.com/matklad/sync_lazy

@BurntSushi

This comment has been minimized.

Copy link
Member

BurntSushi commented Aug 3, 2018

I like the direction this is headed in! Ultimately, I'd really like to see something like this in std due to its ubiquity and utility, so I'm appreciative of any effort towards that end. :-)

With that said, a dependency on parking_lot would probably stop me from using sync_lazy in practice. I think parking_lot is phenomenal, but the increase in the number of transitive dependencies doesn't "feel" proportional to me. Namely, one of my favorite things about lazy_static is that it doesn't bring in a whole bunch of extra dependencies. I would continue using it over sync_lazy for that reason alone.

With that said, this is a double edge sword, because I also like the idea of more people using and testing parking_lot. I think I just tend to be a bit more conservative with these types of things, so overall I like the idea of suggesting sync_lazy as a possible alternative!

@matklad

This comment has been minimized.

Copy link
Author

matklad commented Aug 3, 2018

With that said, a dependency on parking_lot would probably stop me from using sync_lazy in practice.

That is a valid concern. We need parking_lot for a Once::call_once which works for non-static Onces. This is coming to std in rust-lang/rust#52239 (currently beta, I believe). So we can make parking_lot an optional (and probably default) dependency, and otherwise use ::std::sync::Once. That is, this exposes a tradeoff between "more dependencies" and "older rustc".

Now, rust-lang/rust#52239 literally just removed the 'static bound, without changing implementation at all. So, I think we can also polyfill that for older rusts by just transmuting from &'a Once to &'static Once?

So, two questions:

  • how do you feel about making parking_lot default?
  • how do you feel about faking a 'static lifetime?
@BurntSushi

This comment has been minimized.

Copy link
Member

BurntSushi commented Aug 3, 2018

I'm not sure about the 'static thing. I'd have to go understand what's actually happening. :-)

In terms of making parking_lot the default... Not sure. lazy_static is often used as an internal dependency, so folks would need to re-export that feature all the way up the dependency chain, right? That kind of sounds like a drag, but honestly, it does sound like a fairly decent compromise to me.

@matklad

This comment has been minimized.

Copy link
Author

matklad commented Aug 3, 2018

so folks would need to re-export that feature all the way up the dependency chain, right?

Yep, that is correct. OTOH, if you care about number of deps, your deps are probably caring about that as well, and would use sync_lazy with parking-lot disabled/reexported :-) Also, because Lazy does not have to be static, you can have a large number of Lazy at runtime, so at least opt-in into potentially more efficient synchronization primitives is desired.

I've pushed updates that make parking_lot optional and default, employ the &'static "polyfill" for Once and remove usages of non-essential new features.

That gives use compatibility with Rust 1.24.0 an up, with or without parking-lot, on a somewhat questionable ground that pretending that non-static Once is static is OK :)

@anp

This comment has been minimized.

Copy link
Contributor

anp commented Aug 3, 2018

Cool! Do the fields here need to be public? We just finished removing the equivalent from lazy_static since it's technically a very tiny soundness problem.

@matklad

This comment has been minimized.

Copy link
Author

matklad commented Aug 3, 2018

Excellent observation @anp! I can't think of a way to make fields private: because we don't create a fresh type for per lazy instance anymore, we need to store the closure inside a struct, and that should work in const context.

What we can do, however, is to hide Once behind a type with empty public interface. That should fix the soundness issue. Implemented in matklad/sync_lazy@dedd4b5.

@matklad

This comment has been minimized.

Copy link
Author

matklad commented Aug 3, 2018

No, this doesn't entirely fix the hole unfortunately, the user can just overwrite the state directly :( What we can do is to split the State enum which stores either a T or an F into two options, and make the first one private. I am not quite sure its worth the effort to do so: current representation is more natural, and I don't think that subverting soundness via clearly private implementation details really counts. The proper fix here, of course, is to stabilize const_fn :)

@matklad

This comment has been minimized.

Copy link
Author

matklad commented Aug 4, 2018

Another potential problem with Lazys approach is that we have to use fn() -> T instead of impl FnOnce() -> T to be able to spell out a type for a static. This requires us to actually store the function pointer, thus Lazy is one pointer larger than a lazy static:

HashMap<i32, String> ()
lazy_static 48 16
sync_lazy 56 24

There's also an extra indirection during intialization, but that probably does not matter, because Once::call_once is cold and, in fact, already an indirect call.

@clarfon

This comment has been minimized.

Copy link

clarfon commented Sep 27, 2018

I personally wouldn't mind if Lazy<T> became Lazy<T, F>. It'd require existential types but ultimately work better. You could also write Lazy<T, fn() ->T> to pointer stored if need be.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.