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

perf: Replace HashMap with a Vec #241

Merged
merged 1 commit into from
Nov 10, 2023
Merged

Conversation

ChrisDenton
Copy link
Contributor

This may be a minor startup perf win (as env_logger does not need the expense of DoS resistant hashes) but my main reason for doing this is that the default hashing algorithm depends on random number generation, which may fail. This is important to avoid especially because env_logger is often used on program startup. This was a real issue on Windows. While that specific issue is now fixed, it would be nice to avoid this class of issues if possible.

Copy link
Member

@matthiasbeyer matthiasbeyer left a comment

Choose a reason for hiding this comment

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

Hi!

I have a question (read below).

Maybe also: Can you insert the rationale from your PR message in the commit message? This way people finding this to be a Vec instead of a HashMap don't have to search github to find why it was written this way!

👍

src/filter/mod.rs Outdated Show resolved Hide resolved
This may be a minor startup perf win (as `env_logger` does not need the
expense of DoS resistant hashes) but my main reason for doing this is
that the default hashing algorithm depends on random number generation,
which may fail.
This is important to avoid especially because `env_logger` is often used
on program startup.
This was a real issue on Windows.
While that specific issue is now fixed,
it would be nice to avoid this class of issues if possible.
@epage epage changed the title Replace HashMap with a Vec perf: Replace HashMap with a Vec Nov 10, 2023
@epage epage requested a review from matthiasbeyer November 10, 2023 15:08
@epage epage merged commit 9df7e6c into rust-cli:main Nov 10, 2023
13 checks passed
@ChrisDenton ChrisDenton deleted the simple-insert branch November 10, 2023 15:26
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.

3 participants