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

Make rehashing and resizing less generic #282

Merged
merged 3 commits into from
Jul 21, 2021
Merged

Conversation

Zoxc
Copy link
Contributor

@Zoxc Zoxc commented Jul 20, 2021

This makes the code in rehash_in_place, resize and reserve_rehash less generic on T. It also improves the performance of rustc. That performance increase in partially attributed to the use of #[inline(always)].

This is the effect on rustc runtime:

clap:check                        1.9523s   1.9327s  -1.00%
hashmap-instances:check           0.0628s   0.0624s  -0.57%
helloworld:check                  0.0438s   0.0436s  -0.50%
hyper:check                       0.2987s   0.2970s  -0.59%
regex:check                       1.1497s   1.1402s  -0.82%
syn:check                         1.7004s   1.6851s  -0.90%
syntex_syntax:check               6.9232s   6.8546s  -0.99%
winapi:check                      8.3220s   8.2857s  -0.44%

Total                            20.4528s  20.3014s  -0.74%
Summary                           4.0000s   3.9709s  -0.73%

rustc_driver's code size is increased by 0.02%.

This is the effect on compile time this has on my HashMap compile time benchmark:

hashmap-instances:check           0.0636s   0.0632s  -0.61%
hashmap-instances:release        33.0166s  32.2487s  -2.33%
hashmap-instances:debug           7.8677s   7.2012s  -8.47%

Total                            40.9479s  39.5131s  -3.50%
Summary                           1.5000s   1.4430s  -3.80%

The hashmap-instances:debug compile time could be further improved if there was a way to apply #[inline(always)] only on release builds.

@@ -1312,6 +1224,19 @@ impl<A: Allocator + Clone> RawTableInner<A> {
Bucket::from_base_index(self.data_end(), index)
}

#[cfg_attr(feature = "inline-more", inline)]
unsafe fn bucket_ptr(&self, index: usize, size_of: usize) -> *mut u8 {
Copy link
Member

Choose a reason for hiding this comment

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

This function absolutely needs to be #[inline] at minimum, it is not generic.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is generic over A, so I figured this should match the inlining of fn bucket. Though I would put #[inline(always)] on both of them (on release mode).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's a bunch of small functions which are beneficial to inline. I'll open a PR for those. It seems the compile time benefit of #119 mostly comes from removing inline on get, insert, etc.

src/raw/mod.rs Outdated Show resolved Hide resolved
// that we haven't rehashed yet. We unfortunately can't preserve the
// element since we lost their hash and have no way of recovering it
// without risking another panic.
self.prepare_rehash_in_place();
Copy link
Member

Choose a reason for hiding this comment

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

There's no reason for prepare_rehash_in_place to be a separate function any more now that rehash_in_place is moved to RawTableInner.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I find rehash_in_place to be sufficiently large and complex already, so I don't mind this being in a separate function for code clarity. I'm less sure about prepare_resize staying a separate function though.

src/raw/mod.rs Outdated Show resolved Hide resolved
src/raw/mod.rs Outdated Show resolved Hide resolved
@Amanieu
Copy link
Member

Amanieu commented Jul 21, 2021

@bors r+

@bors
Copy link
Collaborator

bors commented Jul 21, 2021

📌 Commit bf8635d has been approved by Amanieu

@bors
Copy link
Collaborator

bors commented Jul 21, 2021

⌛ Testing commit bf8635d with merge 4aca8e4...

@bors
Copy link
Collaborator

bors commented Jul 21, 2021

☀️ Test successful - checks-actions
Approved by: Amanieu
Pushing 4aca8e4 to master...

@bors bors merged commit 4aca8e4 into rust-lang:master Jul 21, 2021
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

3 participants