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.

Sign up for GitHub

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

Jump to bottom

chore(rust): remove wasm-timer dependency #10347

Merged
merged 2 commits into from
Aug 7, 2023
Merged

chore(rust): remove wasm-timer dependency #10347

merged 2 commits into from
Aug 7, 2023

Conversation

orlp
Copy link
Collaborator

@orlp orlp commented Aug 7, 2023

Fixes #10314.

ritchie46
ritchie46 approved these changes Aug 7, 2023
View reviewed changes
crates/polars-core/src/chunked_array/logical/categorical/stringcache.rs Outdated
@@ -16,6 +15,7 @@ use crate::prelude::InitHashMaps;
/// to determine how many threads use the string cache
/// if the refcount is zero, we may clear the string cache.
pub(crate) static USE_STRING_CACHE: AtomicU32 = AtomicU32::new(0);
static STRING_CACHE_UUID_CTR: AtomicU64 = AtomicU64::new(0);
Copy link
Member

Choose a reason for hiding this comment

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

Nice solution!

Copy link
Member

Choose a reason for hiding this comment

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

Any benefit if we shrink to u32? Not really here, I think

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't know how/when it's used enough to give meaningful feedback. The original was a u128 so I wanted to be conservative with how much I shrink it.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, that was a naive solution because I just took time.

We will never have 4 billion unique string cache inits, so u32 will be large enough.

@github-actions github-actions bot added internal An internal refactor or improvement rust Related to Rust Polars labels Aug 7, 2023
@ritchie46 ritchie46 merged commit 2f9822e into pola-rs:main Aug 7, 2023
23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ritchie46 ritchie46 ritchie46 approved these changes

@stinodego stinodego Awaiting requested review from stinodego

@alexander-beedie alexander-beedie Awaiting requested review from alexander-beedie alexander-beedie is a code owner

Assignees
No one assigned
Labels
internal An internal refactor or improvement rust Related to Rust Polars
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Replace unmaintained wasm-timer dependency
2 participants
@orlp @ritchie46