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

Replace Hashmaps with FxHashmaps or another data structure #2487

Closed
Ethan-000 opened this issue Aug 30, 2023 · 4 comments · Fixed by #2490
Closed

Replace Hashmaps with FxHashmaps or another data structure #2487

Ethan-000 opened this issue Aug 30, 2023 · 4 comments · Fixed by #2490
Labels
enhancement New feature or request

Comments

@Ethan-000
Copy link
Contributor

Ethan-000 commented Aug 30, 2023

Problem

In some programs hashmaps is taking a lot of time

below is the flamegraph of the tally circuit

1

Happy Case

FxHashmaps is what the rust compiler uses internally, which should speed it up

Alternatives Considered

or another datastructure

Additional Context

No response

Would you like to submit a PR for this Issue?

Maybe

Support Needs

No response

@Ethan-000 Ethan-000 added the enhancement New feature or request label Aug 30, 2023
@github-project-automation github-project-automation bot moved this to 📋 Backlog in Noir Aug 30, 2023
@Ethan-000
Copy link
Contributor Author

its not improving as much as id think

hashmap

not_using_fx

fxhash

using_fx

@Ethan-000 Ethan-000 changed the title Replace Hashmaps with FxHashmaps Replace Hashmaps with FxHashmaps or another data strcuture Aug 30, 2023
@jfecher
Copy link
Contributor

jfecher commented Aug 30, 2023

It's a bit of an improvement still I suppose, and probably not hard to implement. Did you replace the HashMaps of that one function or all through SSA / the compiler?

(edit: there's a PR now)

@kevaundray kevaundray changed the title Replace Hashmaps with FxHashmaps or another data strcuture Replace Hashmaps with FxHashmaps or another data structure Aug 30, 2023
@kevaundray
Copy link
Contributor

We don't have a tracking issue for benchmarks so putting it here: I wonder how much time we are spending in the arena, here:

pub use generational_arena::{Arena, Index};

If its a lot, we could replace that with something simpler like la-arena which should be faster as we are not using the generational component of generational-arena

@jfecher
Copy link
Contributor

jfecher commented Aug 30, 2023

We can just replace the arenas with Vecs honestly. We never delete elements anyway

@github-project-automation github-project-automation bot moved this from 📋 Backlog to ✅ Done in Noir Sep 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

3 participants