Open TheRealLorenz opened 6 months ago
Should I migrate the jit
mod? It's not currently included in the project
Should I migrate this commented out blocks?
Should I migrate this commented out blocks?
Anything that is commented out or not included you can skip
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 https://github.com/mattwparas/steel/pull/165#discussion_r1485192860:
- query_parameters: HashMap<String, String>,
- query_parameters: FxHashMap<String, String>,
Can we leave these the same?
In crates/steel-core/src/rvals.rs https://github.com/mattwparas/steel/pull/165#discussion_r1485193204:
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 https://github.com/mattwparas/steel/pull/165#discussion_r1485193360:
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 https://github.com/mattwparas/steel/pull/165#discussion_r1485193787:
- 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 https://github.com/mattwparas/steel/pull/165#pullrequestreview-1873738135, or unsubscribe https://github.com/notifications/unsubscribe-auth/AOE5M75CXI7V224WH7KDBULYS6PLBAVCNFSM6AAAAABDB5NMH2VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTQNZTG4ZTQMJTGU . You are receiving this because you authored the thread.Message ID: @.***>
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
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.
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
Closes #92
steel-gen
is missing)Should I also migrate files that are not currently included in the project? Like, the
jit
mod understeel-core
.