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

Optimizing rustc: replace HashMap with OrderMap? #45273

Closed
ghost opened this issue Oct 14, 2017 · 4 comments
Closed

Optimizing rustc: replace HashMap with OrderMap? #45273

ghost opened this issue Oct 14, 2017 · 4 comments
Labels
C-enhancement Category: An issue proposing an enhancement or a PR with one. I-compiletime Issue: Problems and improvements with respect to compile times. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@ghost
Copy link

ghost commented Oct 14, 2017

Just for fun, I tried changing

pub type FxHashMap<K, V> = HashMap<K, V, BuildHasherDefault<FxHasher>>;

to

pub type FxHashMap<K, V> = OrderMap<K, V, BuildHasherDefault<FxHasher>>;

Using the new compiler, compilation time of my crate went down from 28.24 seconds to 25.46 seconds. I didn't do any further benchmarks yet, but the results seem promising. Looks like we could easily shave 5-10% from compilation time simply by changing the hash map implementation.

I was wondering if anyone else considered doing the same, and whether this is something I should experiment with further? What do you think?

cc @bluss

@kennytm kennytm added C-enhancement Category: An issue proposing an enhancement or a PR with one. I-compiletime Issue: Problems and improvements with respect to compile times. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Oct 14, 2017
@kennytm
Copy link
Member

kennytm commented Oct 14, 2017

Perhaps you could submit a PR, which we could do a benchmark using perf.rust-lang.org (cc @Mark-Simulacrum).

@bluss
Copy link
Member

bluss commented Oct 14, 2017

To tell the most important parts of the context for OrderMap as I understand it.

  • Its key-value pairs are stored compactly
    • Pro iteration is just as fast as iterating &[(u64, K, V)]
  • Its hash buckets and its key-value pairs are stored in two separate Vecs
    • Pro Less data to move in remove
    • Pro Less data to move when resizing
    • Pro More buckets fit in cache when scanning for lookup
    • Con Lookup always touches two places in memory even in the best case. Once in the hash buckets, and once in the separate k-v vector
  • OrderMap grows at a load factor of 75%
    • Pro Simpler code / faster lookup
    • Con More memory usage
  • OrderMap does not fully implement its reserve method. It can grow correctly, it can be preallocated with with_capacity correctly, but not more.

There are some systemic factors here like the separation of buckets and k-v entries; ordermap should not be able to compete with HashMap's lookup speed, if the data is not in cache.

I think this means we must look at larger compile jobs to draw conclusions.

bors added a commit that referenced this issue Oct 14, 2017
[Experiment] Replace HashMap with OrderMap

Do not merge.

This is just a simple experiment where I modified `FxHashMap` to use `OrderMap` instead of `HashMap`, as explained in #45273. Don't expect the code to look good. :)

cc @Mark-Simulacrum - Can we please run performance tests to see how this PR impacts compile times?
cc @bluss @kennytm

@eddyb said on IRC that we shouldn't blindly swap the implementation just yet - let's investigate a bit further. If changing the hash map implementation affects performance a lot, then we can probably gain even more by using a different data structure.
@ghost
Copy link
Author

ghost commented Oct 14, 2017

More compilation timings:

crate HashMap OrderMap
crossbeam 31.94 secs 26.44 secs
rayon 9.40 secs 8.20 secs
pdqsort 0.23 secs 0.18 secs
chan 6.16 secs 5.26 secs
string-cache 10.13 secs 9.71 secs
parking_lot 5.45 secs 4.92 secs
timely-dataflow 19.69 secs 16.55 secs

@ghost
Copy link
Author

ghost commented Oct 15, 2017

Closing because we didn't get good results. See #45282.

@ghost ghost closed this as completed Oct 15, 2017
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-enhancement Category: An issue proposing an enhancement or a PR with one. I-compiletime Issue: Problems and improvements with respect to compile times. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

2 participants