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

Force inlining of AttrsIter::next() #152

Merged
merged 1 commit into from
Oct 31, 2016
Merged

Conversation

philipc
Copy link
Collaborator

@philipc philipc commented Oct 30, 2016

Before:
test bench_parsing_debug_info ... bench: 2,423,653 ns/iter (+/- 50,801)
test bench_parsing_debug_info_tree ... bench: 2,767,421 ns/iter (+/- 22,242)

After:
test bench_parsing_debug_info ... bench: 1,521,066 ns/iter (+/- 35,440)
test bench_parsing_debug_info_tree ... bench: 1,740,148 ns/iter (+/- 73,535)

It's a bit worrying that this makes such a large difference.

Before:
test bench_parsing_debug_info      ... bench:   2,423,653 ns/iter (+/- 50,801)
test bench_parsing_debug_info_tree ... bench:   2,767,421 ns/iter (+/- 22,242)

After:
test bench_parsing_debug_info      ... bench:   1,521,066 ns/iter (+/- 35,440)
test bench_parsing_debug_info_tree ... bench:   1,740,148 ns/iter (+/- 73,535)
@coveralls
Copy link

Coverage Status

Coverage remained the same at 93.836% when pulling 8e93980 on philipc:inline into f870889 on gimli-rs:master.

@fitzgen
Copy link
Member

fitzgen commented Oct 31, 2016

It might be worth adding #[inline] to every one of our iterators...

@fitzgen fitzgen merged commit 53c8f11 into gimli-rs:master Oct 31, 2016
@philipc philipc deleted the inline branch November 6, 2016 00:56
@philipc
Copy link
Collaborator Author

philipc commented Nov 6, 2016

It might be worth adding #[inline] to every one of our iterators...

Only if the benchmark shows significant gain. I prefer not to blindly add #[inline], and even in this case I would like to remove the inline again once rustc improves. A large part of the problem is that rustc isn't optimizing large moves (eg see the code generated for https://is.gd/31IOrU, it does 4 moves when it should only need 1). I think this will improve once rustc has MIR passes for inlining and move propagation. There is currently an open PR for inlining, and work on move propagation has been done in the past.

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