Replaces HashSet and HashMap with FxHashSet and FxHashMap #165

    
           // 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 · 2024-02-10T16:34:05Z

Should I migrate this commented out blocks?

steel/crates/steel-core/src/values/contracts.rs

Line 3 in 40cd353

// use std::collections::HashMap;

steel/crates/steel-core/src/steel_vm/contract_checker.rs

Line 4 in 40cd353

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

steel/crates/steel-core/src/steel_vm/vm.rs

Lines 5043 to 5048 in 40cd353

// 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

mattwparas

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

crates/steel-core/src/rvals.rs

crates/steel-core/src/steel_vm/primitives.rs

TheRealLorenz · 2024-02-10T17:06:09Z

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<SteelVal>>); +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: ***@***.***>

This reverts commit 4b45147.

mattwparas · 2024-02-10T17:58:39Z

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 · 2024-02-11T10:57:34Z

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 · 2024-02-11T11:01:26Z

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

TheRealLorenz added 5 commits February 9, 2024 19:24

Migrate steel-core to FxHash

3c88076

Migrate steel-language-server to FxHash

4a62e49

Migrate steel-derive to FxHash

46d2fb8

Migrate steel-doc to FxHash

0348820

Remove TODO comment

5b9e21d

TheRealLorenz added 2 commits February 10, 2024 11:07

Migrate steel-webserver

4b45147

steel-core: use FxBuildHasher in ImmutableHashMap

1fce956

mattwparas reviewed Feb 10, 2024

View reviewed changes

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 added 2 commits February 10, 2024 18:30

Revert "Migrate steel-webserver"

583bb3b

This reverts commit 4b45147.

Remove FxBuildHasher from SteelHashSet

309d581

TheRealLorenz added 2 commits February 10, 2024 19:13

Remove FxBuildHasher from SteelHashMap

9a3e238

Remove MutableHashTable shouldn't be Immutable

9d285eb

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replaces HashSet and HashMap with FxHashSet and FxHashMap #165

Replaces HashSet and HashMap with FxHashSet and FxHashMap #165

TheRealLorenz commented Feb 9, 2024 •

edited

TheRealLorenz commented Feb 10, 2024 •

edited

TheRealLorenz commented Feb 10, 2024 •

edited

mattwparas commented Feb 10, 2024

mattwparas left a comment

TheRealLorenz commented Feb 10, 2024 via email •

edited

mattwparas commented Feb 10, 2024

TheRealLorenz commented Feb 11, 2024 •

edited

TheRealLorenz commented Feb 11, 2024 •

edited

Replaces HashSet and HashMap with FxHashSet and FxHashMap #165

Are you sure you want to change the base?

Replaces HashSet and HashMap with FxHashSet and FxHashMap #165

Conversation

TheRealLorenz commented Feb 9, 2024 • edited

TheRealLorenz commented Feb 10, 2024 • edited

TheRealLorenz commented Feb 10, 2024 • edited

mattwparas commented Feb 10, 2024

mattwparas left a comment

Choose a reason for hiding this comment

TheRealLorenz commented Feb 10, 2024 via email • edited

mattwparas commented Feb 10, 2024

TheRealLorenz commented Feb 11, 2024 • edited

Benchmarks

TheRealLorenz commented Feb 11, 2024 • edited

TheRealLorenz commented Feb 9, 2024 •

edited

TheRealLorenz commented Feb 10, 2024 •

edited

TheRealLorenz commented Feb 10, 2024 •

edited

TheRealLorenz commented Feb 10, 2024 via email •

edited

TheRealLorenz commented Feb 11, 2024 •

edited

TheRealLorenz commented Feb 11, 2024 •

edited