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

cfi: various cleanups #400

Merged
merged 9 commits into from
Mar 7, 2019
Merged

cfi: various cleanups #400

merged 9 commits into from
Mar 7, 2019

Conversation

philipc
Copy link
Collaborator

@philipc philipc commented Mar 6, 2019

Try to simplify and makes things more consistent. I was looking into this as part of seeing if we could do more for #296, but I didn't come up with anything significant.

The commit I'm most unsure about is eeae767 (Change callbacks to match UnwindSection::cie_from_offset).

There are minor performance improvements:

 name                                                       before ns/iter  after ns/iter  diff ns/iter  diff %  speedup 
 cfi::eval_longest_fde_instructions_new_ctx_everytime       532             546                      14   2.63%   x 0.97 
 cfi::eval_longest_fde_instructions_same_ctx                353             329                     -24  -6.80%   x 1.07 
 cfi::iterate_entries_and_do_not_parse_any_fde              63,038          57,991               -5,047  -8.01%   x 1.09 
 cfi::iterate_entries_and_parse_every_fde                   490,942         450,314             -40,628  -8.28%   x 1.09 
 cfi::iterate_entries_and_parse_every_fde_and_instructions  1,065,654       1,013,824           -51,830  -4.86%   x 1.05 
 cfi::iterate_entries_evaluate_every_fde                    1,376,872       1,327,763           -49,109  -3.57%   x 1.04 
 cfi::parse_longest_fde_instructions                        224             222                      -2  -0.89%   x 1.01 

@philipc philipc requested a review from fitzgen March 6, 2019 09:14
@philipc
Copy link
Collaborator Author

philipc commented Mar 6, 2019

Another thing to consider is the use of Box for UninitializedUnwindContext. Since we now only ever accept it by reference, and don't hold long lived references to it (only in UnwindTable which is normally short lived), maybe we should remove the Box now? Then it would be up to the caller to decide where they want to allocate it.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.009%) to 86.623% when pulling bfc4f72 on philipc:issue-296-2 into b8d7ee7 on gimli-rs:master.

@fitzgen
Copy link
Member

fitzgen commented Mar 6, 2019

Another thing to consider is the use of Box for UninitializedUnwindContext. Since we now only ever accept it by reference, and don't hold long lived references to it (only in UnwindTable which is normally short lived), maybe we should remove the Box now? Then it would be up to the caller to decide where they want to allocate it.

The original motivation for the box was to avoid moving large structs around on the stack, if I recall correctly. Now that we have switched to a reference-based API instead of a move-based API, this seems like a good change to make.

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.

👍

@philipc philipc merged commit f30aeaa into gimli-rs:master Mar 7, 2019
@philipc philipc deleted the issue-296-2 branch March 7, 2019 02:19
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