statrs-dev / statrs

Statistical computation library for Rust
https://docs.rs/statrs/latest/statrs/
MIT License
544 stars 78 forks source link

Remove lazy-static and make FCACHE a proper const #211

Closed anergictcell closed 2 months ago

anergictcell commented 2 months ago

This minor change removes the lazy_static dependency by making the FCACHE a const.

YeungOnion commented 2 months ago

Dropping another dependency would be nice. Do you know if this will still be static or will it work like inlining?

anergictcell commented 2 months ago

Im not sure, I don't know enough about inlining in Rust.

On Wed, Apr 17, 2024, at 8:45 AM, Orion Yeung wrote:

Dropping another dependency would be nice. Do you know if this will still be static or will it work like inlining?

— Reply to this email directly, view it on GitHub https://github.com/statrs-dev/statrs/pull/211#issuecomment-2060269021, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAGVZN3OTRV4TAESPHSZUP3Y5XSLTAVCNFSM6AAAAABF5J5QISVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDANRQGI3DSMBSGE. You are receiving this because you authored the thread.Message ID: @.***>

FreezyLemon commented 2 months ago

A const is not static in the C/C++ sense of the word. You should expect inlining when you use a const variable.

That said, you don't have to switch to const. static will mostly work the same way (just declare it as static FCACHE) and will instead put it into some memory location in the binary instead of inlining.

const is generally preferable because it allows for more optimizations, but if binary size is a concern or you are otherwise sure you want a static, it's no problem to switch.

YeungOnion commented 2 months ago

Thanks for commenting on this one @FreezyLemon, I didn't know there's more chance of optimizations, so I learned something new.

I think FCACHE appears only twice in the entire crate. An additional [f64; 170] is a lot smaller than the crate. Seems like a great idea to change to const. Will merge!