-
Notifications
You must be signed in to change notification settings - Fork 158
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
Inline symbol-related methods #87
Conversation
How did you decide which functions to inline? Inlining functions is fine, just trying to gauge the completeness of this, or whether maybe some of these functions are big enough that maybe they shouldn't be inlined. |
Yea I don't think this many functions need to be inlined to speed up symbol iteration (probably just the try_from is my guess, taken care of in the scroll PR), e.g., |
I just marked functions which had a very simple implementation (e.g. |
@@ -468,6 +483,7 @@ if_alloc! { | |||
type Item = <SymIterator<'a> as Iterator>::Item; | |||
type IntoIter = SymIterator<'a>; | |||
|
|||
#[inline] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the only one that I don't see any benefit for, since it won't be called often and there's little room for optimization.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method only performs 3 copies and 2 zero-initializations, which are practically free if inlined into the parent function. While you are technically correct that this won't be called often, there is absolutely zero downside to marking it as inline.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
Together with m4b/scroll_derive#6, this significantly improves the performance of
SymIterator
.When iterating through an ELF file with 2.5M symbols:
Before: 219ms
After: 120ms