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

Replaces HashSet and HashMap with FxHashSet and FxHashMap #165

Draft
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

TheRealLorenz
Copy link

@TheRealLorenz TheRealLorenz commented Feb 9, 2024

Closes #92

  • Migrate crates under crates/ (steel-gen is missing)
  • Migrate crates under libs/

Should I also migrate files that are not currently included in the project? Like, the jit mod under steel-core.

@TheRealLorenz
Copy link
Author

TheRealLorenz commented Feb 10, 2024

Should I migrate the jit mod? It's not currently included in the project

use std::collections::HashMap;

use std::collections::HashSet;

use std::collections::{HashMap, HashSet};

@TheRealLorenz
Copy link
Author

TheRealLorenz commented Feb 10, 2024

Should I migrate this commented out blocks?

// use std::collections::HashMap;

use std::collections::{BTreeSet, HashMap};

// lazy_static! {
// static ref SUPER_PATTERNS: std::collections::HashMap<
// Vec<(OpCode, Option<usize>)>,
// for<'r> fn(&'r mut VmCore<'_>) -> Result<()>,
// > = create_super_instruction_map();
// }

@mattwparas
Copy link
Owner

Should I migrate this commented out blocks?

// use std::collections::HashMap;

use std::collections::{BTreeSet, HashMap};

// lazy_static! {
// static ref SUPER_PATTERNS: std::collections::HashMap<
// Vec<(OpCode, Option<usize>)>,
// for<'r> fn(&'r mut VmCore<'_>) -> Result<()>,
// > = create_super_instruction_map();
// }

Anything that is commented out or not included you can skip

Copy link
Owner

@mattwparas mattwparas left a comment

Choose a reason for hiding this comment

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

Thanks for the changes! I do think I should have been more clear on "blanket" replace things, since there are some spots we don't want that, however I left some comments on where we don't want the changes.

We should try to run some benchmarks on this as well before merge to make sure we're not introducing any regressions. Once we're ready we can do that

libs/steel-webserver/src/lib.rs Outdated Show resolved Hide resolved
crates/steel-core/src/rvals.rs Outdated Show resolved Hide resolved
crates/steel-core/src/rvals.rs Outdated Show resolved Hide resolved
crates/steel-core/src/steel_vm/primitives.rs Outdated Show resolved Hide resolved
@TheRealLorenz
Copy link
Author

TheRealLorenz commented Feb 10, 2024 via email

@mattwparas
Copy link
Owner

Sorry if I bulk swapped everything, I initially meant to look into every single case, but I figured it would be way faster to swap everything and wait for a review, since I’m not familiar at all with the codebase. I’m applying the corrections as soon as I can Il giorno sab 10 feb 2024 alle 17:41 Matthew Paras @.> ha scritto:

@.
* commented on this pull request. Thanks for the changes! I do think I should have been more clear on "blanket" replace things, since there are some spots we don't want that, however I left some comments on where we don't want the changes. We should try to run some benchmarks on this as well before merge to make sure we're not introducing any regressions. Once we're ready we can do that ------------------------------ In libs/steel-webserver/src/lib.rs <#165 (comment)>: > - query_parameters: HashMap<String, String>, + query_parameters: FxHashMap<String, String>, Can we leave these the same? ------------------------------ In crates/steel-core/src/rvals.rs <#165 (comment)>: > SteelHashMap(value) } } #[derive(Clone, PartialEq)] -pub struct SteelHashSet(pub(crate) Gc<im_rc::HashSet>); +pub struct SteelHashSet(pub(crate) Gc<im_rc::HashSet<SteelVal, FxBuildHasher>>); I realize this was unclear - however I would prefer if these do not take the FxBuildHasher ------------------------------ In crates/steel-core/src/rvals.rs <#165 (comment)>: > SteelVector(value) } } #[derive(Clone, PartialEq)] -pub struct SteelHashMap(pub(crate) Gc<HashMap<SteelVal, SteelVal>>); +pub struct SteelHashMap(pub(crate) Gc<ImmutableHashMap<SteelVal, SteelVal, FxBuildHasher>>); Same here - if we could leave these as the default hasher since these are how hash maps are implemented for values in steel ------------------------------ In crates/steel-core/src/steel_vm/primitives.rs <#165 (comment)>: > - table: HashMap<SteelVal, SteelVal>, + table: ImmutableHashMap<SteelVal, SteelVal, FxBuildHasher>, I think this is an erroneous change - this shouldn't be changed to an immutable hash map — Reply to this email directly, view it on GitHub <#165 (review)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AOE5M75CXI7V224WH7KDBULYS6PLBAVCNFSM6AAAAABDB5NMH2VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTQNZTG4ZTQMJTGU . You are receiving this because you authored the thread.Message ID: @.***>

No worries! I should provide some more detail next time in the issue. Take your time, no rush

@TheRealLorenz
Copy link
Author

TheRealLorenz commented Feb 11, 2024

Benchmarks

  • startup/startup.scm
    std: Time (mean ± σ):      27.9 ms ±   0.2 ms    [User: 24.2 ms, System: 3.0 ms]
    Fx:  Time (mean ± σ):      27.7 ms ±   0.2 ms    [User: 24.0 ms, System: 3.0 ms]
    ---
    % Improvement: 0.72%
  • map/map.scm
    std: Time (mean ± σ):      28.8 ms ±   0.1 ms    [User: 25.0 ms, System: 3.0 ms]
    Fx:  Time (mean ± σ):      28.5 ms ±   0.1 ms    [User: 24.8 ms, System: 3.0 ms]
    ---
    % Improvement: 1.0%
  • ack/ack.scm
    std: Time (mean ± σ):      35.6 ms ±   0.4 ms    [User: 31.8 ms, System: 3.0 ms]
    Fx:  Time (mean ± σ):      35.3 ms ±   0.5 ms    [User: 31.6 ms, System: 3.0 ms]
    ---
    % Improvement: 0.84%
  • fib/fib.scm
    std: Time (mean ± σ):      98.4 ms ±  13.6 ms    [User: 94.5 ms, System: 3.1 ms]
    Fx:  Time (mean ± σ):      95.5 ms ±   4.2 ms    [User: 91.6 ms, System: 3.0 ms]
    ---
    % Improvement: 2.9%
  • bin-trees/bin-trees.scm
    std: Time (mean ± σ):     305.3 ms ±   1.5 ms    [User: 296.9 ms, System: 6.9 ms]
    Fx:  Time (mean ± σ):     302.4 ms ±   0.7 ms    [User: 294.2 ms, System: 6.6 ms]
    ---
    % Improvement: 0.95%

Unfortunately, I couldn't get rid of:

Warning: Statistical outliers were detected. Consider re-running this benchmark on a quiet system without any interferences from other programs. It might help to use the '--warmup' or '--prepare' options.

@TheRealLorenz
Copy link
Author

TheRealLorenz commented Feb 11, 2024

The other benches seems like they're disabled. Should I run them too? Otherwise, I'm rebasing against master and marking this PR as ready for review

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.

Replace usages of std::collections::HashMap + HashSet with FxHashMap/FxHashSet
2 participants