Simplify HashMap layout calculation by using Layout

[Amanieu]

RawTable uses a single allocation to hold both the array of hashes and the array of key/value pairs. This PR changes RawTable to use Layout when calculating the amount of memory to allocate instead of performing the calculation manually.

r? @SimonSapin

[gnzlbg]

LGTM!

[SimonSapin]

@bors r+

[bors]

📌 Commit 0fde52b has been approved by SimonSapin

[bors]

☔ The latest upstream changes (presumably #50880) made this pull request unmergeable. Please resolve the merge conflicts.

[Amanieu]

Rebased.

[SimonSapin]

@bors r+

[bors]

📌 Commit 01ba7cb has been approved by SimonSapin

[bors]

🔒 Merge conflict

[Amanieu]

Rebased again.

@SimonSapin I think this may conflict with your stabilization PR since I added impl From<LayoutErr> for CollectionAllocErr to libcore/alloc.rs.

[SimonSapin]

@Amanieu I don’t mind the conflict, I’ll deal with it in the other PR.

But that impl might be notable with respect to stability. If CollectionAllocErr stays unstable but LayoutErr is stabilized, will (x: LayoutErr).into() allow stable code to obtain a CollectionAllocErr, making it to some extent de-facto stable?

[SimonSapin]

@bors r+

[bors]

📌 Commit c6bebf4 has been approved by SimonSapin

[bors]

⌛ Testing commit c6bebf4 with merge aa094a4...

[bors]

☀️ Test successful - status-appveyor, status-travis
Approved by: SimonSapin
Pushing aa094a4 to master...

[andjo403]

noticed that this PR have a lot of big red numbers in the perf run maybe there is something that can be improved?

[Amanieu]

Hmm, that's worrying. I can think of three ways to improve performance:

• A lot of the Layout methods aren't implemented very efficiently, they could be improved.
• A lot of the Layout methods aren't marked as #[inline], which hurts us when the inputs are compile-time constants (size_of, align_of).
• We could cache the pointer to the (K, V) array in the RawTable rather than recalculating it every time.

[gnzlbg]

Amanieu my NonZeroUsize PR marks them as inline but I did not see many obvious perf improvements there. Maybe you could take a look there?

…

On Sat 2. Jun 2018 at 13:07, Amanieu ***@***.***> wrote: Hmm, that's worrying. I can think of two ways to improve performance: - A lot of the Layout methods aren't implemented very efficiently, they could be improved. - A lot of the Layout methods aren't marked as #[inline], which hurts us when the inputs are compile-time constants (size_of, align_of). - We could cache the pointer to the (K, V) array in the RawTable rather than recalculating it every time. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#51163 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AA3Nplh0ZoNrzIIJB4klIEx9nYS2zGeBks5t4nHVgaJpZM4URTRe> .

[Amanieu]

I think the main issue here is that Layout::extend needs to be marked as inlined.

I made some changes in a branch but I think that it would be easier if you just added those changes to your PR to avoid conflicts.

[gnzlbg]

@Amanieu the last commit in #51226 contains the changes from your branch, could you triplecheck those?

[nnethercote]

noticed that this PR have a lot of big red numbers in the perf run maybe there is something that can be improved?

I found this independently today and worked back to this PR.

There was a subsequent improvement here:

http://perf.rust-lang.org/compare.html?start=251ec62a134824c1b8c01671a2d05aa793839232&end=3575be60eab140e69e5a75fe5c3b4119c2a17179&stat=instructions:u

which points to these changes:

251ec62...3575be6

But the improvement wasn't enough to get back all the perf that was lost.

Can this PR be backed out? I'd say it has undone roughly 1/3 to 1/2 of the significant rustc perf improvement that had occurred in past few weeks, which is highly discouraging.

cc @Mark-Simulacrum

[nnethercote]

Here is a Cachegrind diff comparing a rustc from May 27 against one from Jun 04. The numbers are instruction counts:

293,637,556 /home/njn/moz/rustN/src/libcore/alloc.rs:<std::collections::hash::map::HashMap<K, V, S>>::get
199,291,681 /home/njn/moz/rustN/src/libstd/collections/hash/map.rs:<std::collections::hash::map::HashMap<K, V, S>>::get
165,729,516 /home/njn/moz/rustN/src/libstd/collections/hash/table.rs:<std::collections::hash::map::HashMap<K, V, S>>::get
159,843,803 /home/njn/moz/rustN/src/libcore/num/mod.rs:<std::collections::hash::map::HashMap<K, V, S>>::get
85,460,589 /home/njn/moz/rustN/src/libstd/collections/hash/set.rs:<std::collections::hash::set::HashSet<T, S>>::get
83,626,730 /home/njn/moz/rustN/src/libcore/alloc.rs:rustc::ty::context::TypeckTables::node_id_to_type_opt
-82,546,355 /home/njn/moz/rustN/src/libstd/collections/hash/map.rs:rustc::middle::region::ScopeTree::temporary_scope
71,081,752 /home/njn/moz/rustN/src/libstd/collections/hash/table.rs:<std::collections::hash::set::HashSet<T, S>>::get
67,990,342 /home/njn/moz/rustN/src/libstd/collections/hash/map.rs:<std::collections::hash::set::HashSet<T, S>>::get
63,276,945 /home/njn/moz/rustN/src/libcore/num/mod.rs:<std::collections::hash::set::HashSet<T, S>>::get
-62,391,972 /home/njn/moz/rustN/src/libsyntax/parse/parser.rs:syntax::parse::parser::Parser::check_keyword
60,819,440 /home/njn/moz/rustN/src/libcore/num/mod.rs:rustc::ty::context::TypeckTables::node_id_to_type_opt
-60,538,623 /home/njn/moz/rustN/src/libcore/num/mod.rs:rustc::middle::region::ScopeTree::temporary_scope
57,140,958 /home/njn/moz/rustN/src/libcore/alloc.rs:<std::collections::hash::map::HashMap<K, V, S>>::try_resize
-51,742,844 /home/njn/moz/rustN/src/libstd/collections/hash/table.rs:rustc::middle::region::ScopeTree::temporary_scope
51,120,900 /home/njn/moz/rustN/src/librustc/middle/region.rs:rustc::middle::region::ScopeTree::temporary_scope
49,415,795 /home/njn/moz/rustN/src/libstd/collections/hash/table.rs:rustc::ty::context::TypeckTables::node_id_to_type_opt

It's mostly additional hash table operations, perhaps involving node_id_to_type_opt.