Skip to content

Optimize find #279

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

Merged
merged 3 commits into from
Jul 15, 2021
Merged

Optimize find #279

merged 3 commits into from
Jul 15, 2021

Conversation

Zoxc
Copy link
Contributor

@Zoxc Zoxc commented Jul 15, 2021

This optimizes find for size and performance.

The cargo llvm-lines output from the following program is reduced from 15837 to 15771.

fn main() {
    let mut map1 = hashbrown::HashMap::new();
    map1.insert(1u8, "");
    map1.reserve(1000);
    dbg!(map1.get(&1));
    let mut map2 = hashbrown::HashMap::new();
    map2.insert(1i16, "");
    map2.reserve(1000);
    dbg!(map2.get(&1));
    let mut map3 = hashbrown::HashMap::new();
    map3.insert(3u16, "");
    map3.reserve(1000);
    dbg!(map3.get(&1));
    let mut map4 = hashbrown::HashMap::new();
    map4.insert(3u64, "");
    map4.reserve(1000);
    dbg!(map4.get(&1));
    dbg!((
        map1.iter().next(),
        map2.iter().next(),
        map3.iter().next(),
        map4.iter().next()
    ));
}

rustc_driver size (with optimizations) is reduced by 0.14%.

Impact on compiler performance:

clap:check                        1.9602s   1.9407s  -0.99%
helloworld:check                  0.0420s   0.0420s  +0.06%
hyper:check                       0.2977s   0.2968s  -0.30%
regex:check                       1.1573s   1.1475s  -0.85%
syn:check                         1.6993s   1.6870s  -0.73%
syntex_syntax:check               6.8989s   6.8263s  -1.05%
winapi:check                      8.4423s   8.3300s  -1.33%

Total                            20.4977s  20.2702s  -1.11%
Summary                           3.5000s   3.4740s  -0.74%

Copy link
Member

@Amanieu Amanieu left a comment

Choose a reason for hiding this comment

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

I'm surprised that you're even seeing a performance difference. I would have expected the generated code to be identical after this change.

src/raw/mod.rs Outdated
@@ -1054,6 +1053,7 @@ impl<T, A: Allocator + Clone> RawTable<T, A> {
/// `RawIterHash`. Because we cannot make the `next` method unsafe on the
/// `RawIterHash` struct, we have to make the `iter_hash` method unsafe.
#[cfg_attr(feature = "inline-more", inline)]
#[allow(dead_code)] // Used when the `raw` API is enabled
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
#[allow(dead_code)] // Used when the `raw` API is enabled
#[cfg(feature = "raw")]

src/raw/mod.rs Outdated
@@ -1255,6 +1255,33 @@ impl<A: Allocator + Clone> RawTableInner<A> {
}
}

/// Searches for an element in the table.
Copy link
Member

Choose a reason for hiding this comment

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

Add an explanation that this is split into a separate function to improve compilation time but has no performance impact from dynamic dispatch in practice due to inlining.

@Amanieu
Copy link
Member

Amanieu commented Jul 15, 2021

Looking at the disassembly in more detail, it seems that the iterator was confusing LLVM somehow which caused it to unnecessarily unroll the loop one extra time.

@Zoxc
Copy link
Contributor Author

Zoxc commented Jul 15, 2021

I was expecting this to only affect debug builds, but I'll take some free performance.

@Amanieu
Copy link
Member

Amanieu commented Jul 15, 2021

@bors r+

@bors
Copy link
Contributor

bors commented Jul 15, 2021

📌 Commit 0c528ef has been approved by Amanieu

@bors
Copy link
Contributor

bors commented Jul 15, 2021

⌛ Testing commit 0c528ef with merge d5002fa...

@bors
Copy link
Contributor

bors commented Jul 15, 2021

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

@bors bors merged commit d5002fa into rust-lang:master Jul 15, 2021
@Zoxc Zoxc deleted the find-opt branch July 15, 2021 17:10
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.

3 participants