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

chore: move lazy_static to once_cell, and some clippy fix #380

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

attila-lin
Copy link

No description provided.

@jeertmans
Copy link
Collaborator

Hello @attila-lin, could you please provide some motivation why changes from lazy_static to once_cell`?

Also, I assume you ran Clippy with the Nightly toolchain?

@attila-lin
Copy link
Author

attila-lin commented Feb 21, 2024

Hello @attila-lin, could you please provide some motivation why changes from lazy_static to once_cell`?

maintenance = { status = "passively-maintained" }

this crate is passively-maintained. The std choose the once_cell impl.

Also, I assume you ran Clippy with the Nightly toolchain?

Yes.

@jeertmans
Copy link
Collaborator

jeertmans commented Feb 21, 2024

Hello @attila-lin, could you please provide some motivation why changes from lazy_static to once_cell`?

maintenance = { status = "passively-maintained" }

this crate is passively-maintained. The std choose the once_cell impl.

The fact that it is passively maintained is not an issue to me, but the second argument looks ok. But I assume we would go for std::cell::LazyCell once rust-lang/rust#109736 is merged (so stabilized).

Also, I assume you ran Clippy with the Nightly toolchain?

Yes.

Fair enough

@jeertmans
Copy link
Collaborator

Benchmarks show a large decreasing in performances, which is surprising:

group                                         before                                 changes
-----                                         ------                                 -------
count_ok/identifiers                          1.06    866.3±5.20ns   857.6 MB/sec    1.00   818.1±20.90ns   908.1 MB/sec
count_ok/keywords_operators_and_punctators    1.00      2.5±0.05µs   805.5 MB/sec    1.02      2.6±0.04µs   790.1 MB/sec
count_ok/strings                              1.00   554.8±12.18ns  1497.3 MB/sec    1.24   689.4±33.73ns  1204.9 MB/sec
iterate/identifiers                           1.02   869.3±19.49ns   854.6 MB/sec    1.00    850.5±4.32ns   873.5 MB/sec
iterate/keywords_operators_and_punctators     1.05      2.7±0.02µs   762.7 MB/sec    1.00      2.5±0.02µs   799.9 MB/sec
iterate/strings                               1.00   585.3±21.16ns  1419.1 MB/sec    1.22   716.3±23.00ns  1159.6 MB/sec

I just restarted the benchmarks, to see if that stays the same.

@attila-lin
Copy link
Author

interesting!

@jeertmans
Copy link
Collaborator

This is very surprising... Do you observe the same benchmark changes locally?

Copy link
Owner

@maciejhirsz maciejhirsz left a comment

Choose a reason for hiding this comment

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

No idea why any of this would affect benchmarks.

use std::convert::TryFrom;

use lazy_static::lazy_static;
use once_cell::sync::Lazy;
Copy link
Owner

Choose a reason for hiding this comment

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

unsync::Lazy is probably a better choice here.

@attila-lin
Copy link
Author

let me try unsync version!

@jeertmans
Copy link
Collaborator

Any update @attila-lin?

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.

4 participants