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

Switch hash table and maps to use FxHash instead of Fnv. #1538

Closed
wants to merge 1 commit into from

Conversation

glennw
Copy link
Member

@glennw glennw commented Aug 1, 2017

Also introduce a FastHashMap and FastHashSet type alias
that is used to allow easily benchmarking different hash
functions, and should be used for all HashMaps and HashSets
that are used by WR.

Some representative benchmarks below showing the total backend
CPU time for various hash types:

Wrench text benchmark
Sip: 2.06 ms
Fnv: 1.32 ms
Fx: 1.12 ms

https://news.ycombinator.com
Sip: 1.40 ms
Fnv: 1.02 ms
Fx: 0.92 ms

https://en.wikipedia.org/wiki/Rust
Sip: 3.14 ms
Fnv: 2.72 ms
Fx: 2.39 ms

@glennw
Copy link
Member Author

glennw commented Aug 1, 2017

r? @kvark

I still want to work on reducing the amount of hashing we do, but this is a significant and easy win we can get right now.

@@ -11,10 +11,10 @@ fn write_shaders(glsl_files: Vec<PathBuf>, shader_file_path: &Path) {
let mut shader_file = File::create(shader_file_path).unwrap();

write!(shader_file, "/// AUTO GENERATED BY build.rs\n\n").unwrap();
write!(shader_file, "use std::collections::HashMap;\n").unwrap();
write!(shader_file, "use internal_types::FastHashMap;\n").unwrap();
Copy link
Member

Choose a reason for hiding this comment

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

This could use PHF, btw. Not sure how hot this lookup is, but given they're static keys seems like it could be better.

Copy link
Member Author

Choose a reason for hiding this comment

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

They're actually only fetched once - when they need to be compiled (lazily) for the first time. So not a high priority, but something we could consider as a follow up.

Also introduce a FastHashMap and FastHashSet type alias
that is used to allow easily benchmarking different hash
functions, and should be used for all HashMaps and HashSets
that are used by WR.

Some representative benchmarks below showing the total backend
CPU time for various hash types:

Wrench text benchmark
 Sip: 2.06 ms
 Fnv: 1.32 ms
 Fx:  1.12 ms

https://news.ycombinator.com
 Sip: 1.40 ms
 Fnv: 1.02 ms
 Fx:  0.92 ms

https://en.wikipedia.org/wiki/Rust
 Sip: 3.14 ms
 Fnv: 2.72 ms
 Fx:  2.39 ms
@emilio
Copy link
Member

emilio commented Aug 2, 2017

@bors-servo r+

@bors-servo
Copy link
Contributor

📌 Commit 49cafde has been approved by emilio

@bors-servo
Copy link
Contributor

⌛ Testing commit 49cafde with merge 74c8614...

bors-servo pushed a commit that referenced this pull request Aug 2, 2017
Switch hash table and maps to use FxHash instead of Fnv.

Also introduce a FastHashMap and FastHashSet type alias
that is used to allow easily benchmarking different hash
functions, and should be used for all HashMaps and HashSets
that are used by WR.

Some representative benchmarks below showing the total backend
CPU time for various hash types:

Wrench text benchmark
 Sip: 2.06 ms
 Fnv: 1.32 ms
 Fx:  1.12 ms

https://news.ycombinator.com
 Sip: 1.40 ms
 Fnv: 1.02 ms
 Fx:  0.92 ms

https://en.wikipedia.org/wiki/Rust
 Sip: 3.14 ms
 Fnv: 2.72 ms
 Fx:  2.39 ms
@emilio
Copy link
Member

emilio commented Aug 2, 2017

@bors-servo r-

Err, forgot that you asked for explicit review by @kvark

@bors-servo
Copy link
Contributor

☀️ Test successful - status-travis
State: approved= try=False

@kvark
Copy link
Member

kvark commented Aug 2, 2017

@emilio I reviewed #1540, which includes this PR

@glennw
Copy link
Member Author

glennw commented Aug 2, 2017

Closing since it's included in #1540 and reviewed as part of that.

@glennw glennw closed this Aug 2, 2017
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.

None yet

5 participants