Skip to content

Some perf tweaks #1473

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 5 commits into from
Dec 29, 2018
Merged

Some perf tweaks #1473

merged 5 commits into from
Dec 29, 2018

Conversation

emilio
Copy link
Contributor

@emilio emilio commented Dec 23, 2018

I did a bit of profiling today, for no good reason. I wanted to get rid of some of our hash maps from before hand so it's good to know that this really helps.

$ time ./bindgen-old tests/stylo.hpp --no-rustfmt-bindings >/dev/null 2>&1                                                                                                                           
./bindgen-old tests/stylo.hpp --no-rustfmt-bindings > /dev/null 2>  6.06s user 0.76s system 98% cpu 6.912 total
$ time ./bindgen-new tests/stylo.hpp --no-rustfmt-bindings >/dev/null 2>&1                                                                                                                                      
./bindgen-new tests/stylo.hpp --no-rustfmt-bindings > /dev/null 2>&1  4.51s user 0.63s system 98% cpu 5.198 total

This isn't such a massive win as I'd have hoped, but it is consistently faster,
so there's no reason not to.

> ./bindgen-old tests/stylo.hpp --no-rustfmt-bindings > /dev/null 2>&1
6.17s user 0.84s system 98% cpu 7.079 total

> ./target/release/bindgen tests/stylo.hpp --no-rustfmt-bindings > /dev/null 2>
5.92s user 0.87s system 98% cpu 6.866 total

Which isn't _that_ much but it's quite a bit.
This wins between 2 and 5 milliseconds more in the test-case above, so no reason
not to I guess.
We use sequential id's so a Vec<Option<T>> does the trick.

This reduces the time for:

  time ./target/release/bindgen tests/stylo.hpp --no-rustfmt-bindings

From ~6s to less than 5s on my machine.
This should allow making it a HashSet.
@emilio
Copy link
Contributor Author

emilio commented Dec 23, 2018

r? @fitzgen

This is bigger than most of the changes that have been around lately, so it'd be amazing if you could take a look.

@emilio
Copy link
Contributor Author

emilio commented Dec 29, 2018

Well, I'll merge this for now, since it's a pretty nice improvement and I want to pull it on mozilla-central. But if you have time to skim over it it'd be also really amazing :)

@emilio emilio merged commit 1104631 into rust-lang:master Dec 29, 2018
@emilio emilio deleted the hash branch December 29, 2018 13:48
Copy link
Member

@fitzgen fitzgen left a comment

Choose a reason for hiding this comment

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

Nice :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants