Changes HashMap to use aHash instead, giving a performance boost. by FilipAndersson245 · Pull Request #9391 · nushell/nushell

FilipAndersson245 · 2023-06-09T14:01:08Z

Description

see #9390
using ahash instead of the default hasher. this will not affect compile time as we where already building ahash.

User-Facing Changes

Tests + Formatting

After Submitting

fdncred · 2023-06-09T14:03:32Z

I'm good with going this direction but I wonder how you arrived at aHash? I mean, how do we know it's the fastest? Could there perhaps be faster ones?

FilipAndersson245 · 2023-06-09T14:05:59Z

I'm good with going this direction but I wonder how you arrived at aHash? I mean, how do we know it's the fastest? Could there perhaps be faster ones?

I had used it previously so i wanted to test if it improved performance, as far as iknow ahash does not lock itself to a specific algorithm so it always uses the fastest one available, it is possible faster ones, but ahash had drop in replacments for the standard library making it easy drop and replace.

edit: maybe there exist faster alternative also depending on if we are okay with the hasher not being DOS resistant.

FilipAndersson245 · 2023-06-09T14:19:00Z

Don't get why the typos gets triggered by this?

fdncred · 2023-06-09T14:57:08Z

it tells you how it's a typo and how to rename it

stormasm · 2023-06-09T15:10:59Z

Don't get why the typos gets triggered by this?

Warning: "reuseable" should be "reusable".

Just send in another commit with this change and it should pass...

stormasm · 2023-06-09T15:28:43Z

@FilipAndersson245

Thank you kindly for investigating this ! Great work 👍

We are going to need to analyze further...

For starters I downloaded your branch and built nushell...

And I am seeing about 5 - 10ms speedup...

This tells me that the benchmarking code may not be the best way to measure the effect of this change.

I would be curious to know what you are seeing on your computer as far as startup times
between your branch and the main branch...

Thanks --- lets investigate further and see what we come up with...

stormasm · 2023-06-09T16:43:47Z

Here are my results in a side by side comparison of the nushell main branch versus the ahash branch.

https://github.com/stormasm/nunotes/blob/main/ahash.md

@FilipAndersson245

In order for me to better understand this stuff can you please create a similar table for me...

Run the nushell main branch parser_benchmarks on your machine.
Then go ahead and run the ahash branch parser_benchmarks on your machine

Thank you !

fdncred · 2023-06-09T20:25:41Z

Just to jump in. Here's me running cargo bench on my m1mac

function	main	ahash
parse_default_env_file	383.47 µs	360.60 µs
parse_default_config_file	1.4435 ms	1.3889 ms
eval default_env.nu	747.83 µs	707.41 µs
eval default_config.nu	1.9011 ms	1.8596 ms
eval default_env.nu 2	750.61 µs	714.26 µs
eval default_config 2	1.8857 ms	1.8774 ms

FilipAndersson245 · 2023-06-09T20:57:03Z

@stormasm

function	main	ahash
parse_default_env_file	315.38 µs	300.96 µs
parse_default_config_file	1.3181 ms	1.1080 ms
eval default_env.nu	664.07 µs	681.50 µs
eval default_config.nu	1.6868 ms	1.6020 ms
eval default_env.nu 2	705.73 µs	648.34 µs
eval default_config.nu 2	1.6323 ms	1.7428 ms

related to - bdb09a9 - [this job](https://github.com/nushell/nushell/actions/runs/5224369668/jobs/9432486180?pr=9392) from #9392 - [this job](https://github.com/nushell/nushell/actions/runs/5222835000/jobs/9428854834?pr=9391) from #9391 # Description this PR tries to fix the typo reported in these very recent PRs

amtoine · 2023-06-10T08:14:53Z

📯 📯 little annoucement

the Typos CI job is failing in this PR.
this has been fixed in #9393 👍

in this PR, you can either

merge the new main branch into the PR branch to explicitely have a ✔️ here
wait for the PR to land and the CI should be ✔️ once landed on the main branch 😌

fdncred · 2023-06-10T15:08:44Z

I'm about ready to land this to see how it goes, but I'd like to have it tested on Windows first. Windows is weird and may behave badly. If someone gets a chance to try it, please post your timings.

stormasm · 2023-06-10T15:36:50Z

I'm about ready to land this to see how it goes, but I'd like to have it tested on Windows first. Windows is weird and may behave badly. If someone gets a chance to try it, please post your timings.

@fdncred I am not sure why we are landing this now...

I ran my tests again this morning just to be sure...

@FilipAndersson245 in two of his ahash tests is causing a slowdown...

And my benchmark tests show a slowdown...

I would feel more comfortable landing this if we had a few other folks run the same tests
we did just for comparison...

As well as look at their startup times / I am not seeing any changes on startup times...

fdncred · 2023-06-10T15:43:17Z

@stormasm it would be good if you could post all 6 of your timings here so we can gather results consistently.

I have 6 of 6 improvements on my mac m1
@FilipAndersson245 has 4 of 6 improvements on linux i assume
@storm has 6 of 6 improvements on mac m1
Darren on Windows 5 of 6 improvements on 32gb lenovo win11 laptop on power

stormasm · 2023-06-10T15:48:13Z

@stormasm it would be good if you could post all 6 of your timings here so we can gather results consistently.

I have 6 of 6 improvements on my mac m1 @FilipAndersson245 has 4 of 6 improvements on linux i assume @storm has 2 of 4 improvements on mac x86 i assume (i'll update this if you show your 6)

I will run them now and post...
Should take a few minutes...
stay tuned.

stormasm · 2023-06-10T16:10:05Z

Apple M1 Pro with 16G of memory

function	main	ahash
parse_default_env_file	411.49 µs	401.16 µs
parse_default_config_file	1.4912 ms	1.4415 ms
eval default_env.nu	823.05 µs	770.71 µs
eval default_config.nu	2.0569 ms	1.9858 ms
eval default_env.nu #2	812.52 µs	767.01 µs
eval default_config.nu #2	2.0333 ms	1.9896 ms

Lets land it ! 😄

fdncred · 2023-06-10T16:26:58Z

i'm building in windows now to see what it does.

fdncred · 2023-06-10T16:40:58Z

Windows

function	main	ahash
parse_default_env_file	958.95 µs	855.95 µs
parse_default_config_file	3.2623 ms	3.2155 ms
eval default_env.nu	2.4146 ms	2.4098 ms
eval default_config.nu	5.5520 ms	5.4936 ms
eval default_env.nu 2	2.6904 ms	2.5983 ms
eval default_config 2	5.0630 ms	5.4408 ms

…ost. (#9391)" This reverts commit 1433f4a.

amtoine · 2023-06-10T16:58:28Z

the revert button creating a commit right away without confirmation is an interesting feature 😅

nothing changed here, no worries 👀

@FilipAndersson245

This PR reverts #9391 We try not to revert PRs like this, though after discussion with the Nushell team, we decided to revert this one. The main reason is that Nushell, as a codebase, isn't ready for these kinds of optimisations. It's in the part of the development cycle where our main focus should be on improving the algorithms inside of Nushell itself. Once we have matured our algorithms, then we can look for opportunities to switch out technologies we're using for alternate forms. Much of Nushell still has lots of opportunities for tuning the codebase, paying down technical debt, and making the codebase generally cleaner and more robust. This should be the focus. Performance improvements should flow out of that work. Said another, optimisation that isn't part of tuning the codebase is premature at this stage. We need to focus on doing the hard work of making the engine, parser, etc better. # User-Facing Changes Reverts the HashMap -> ahash change. cc @FilipAndersson245

Changes HashMap to use aHash instead, giving a performance boost.

3c15e97

fixes windows

1b735a2

amtoine linked an issue Jun 9, 2023 that may be closed by this pull request

Examining alternative hashing algorithms #9390

Closed

amtoine mentioned this pull request Jun 9, 2023

fix a typo #9393

Merged

Merge branch 'nushell:main' into ahash

f4737f6

fdncred merged commit 1433f4a into nushell:main Jun 10, 2023

FilipAndersson245 deleted the ahash branch June 10, 2023 16:48

amtoine added a commit that referenced this pull request Jun 10, 2023

Revert "Changes HashMap to use aHash instead, giving a performance bo…

e08abe2

…ost. (#9391)" This reverts commit 1433f4a.

sophiajt mentioned this pull request Jun 17, 2023

revert: move to ahash #9464

Merged

Conversation

FilipAndersson245 commented Jun 9, 2023

Description

User-Facing Changes

Tests + Formatting

After Submitting

Uh oh!

fdncred commented Jun 9, 2023

Uh oh!

FilipAndersson245 commented Jun 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

FilipAndersson245 commented Jun 9, 2023

Uh oh!

fdncred commented Jun 9, 2023

Uh oh!

stormasm commented Jun 9, 2023

Uh oh!

stormasm commented Jun 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stormasm commented Jun 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fdncred commented Jun 9, 2023

Uh oh!

FilipAndersson245 commented Jun 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

amtoine commented Jun 10, 2023

📯 📯 little annoucement

Uh oh!

fdncred commented Jun 10, 2023

Uh oh!

stormasm commented Jun 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fdncred commented Jun 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stormasm commented Jun 10, 2023

Uh oh!

stormasm commented Jun 10, 2023

Uh oh!

fdncred commented Jun 10, 2023

Uh oh!

fdncred commented Jun 10, 2023

Uh oh!

amtoine commented Jun 10, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

FilipAndersson245 commented Jun 9, 2023 •

edited

Loading

stormasm commented Jun 9, 2023 •

edited

Loading

stormasm commented Jun 9, 2023 •

edited

Loading

FilipAndersson245 commented Jun 9, 2023 •

edited

Loading

stormasm commented Jun 10, 2023 •

edited

Loading

fdncred commented Jun 10, 2023 •

edited

Loading