-
Notifications
You must be signed in to change notification settings - Fork 108
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
Remove smallvec dependency #496
Conversation
No objection to what these PRs are doing, and I'll try to review them tomorrow. This one needs a fix for 32-bit builds though, and that beta SIGSEGV is a concern, but probably not due to this PR? |
Sure thing, and no rush of course! I'll take a look at the errors here. |
13929ba
to
5f1c1b1
Compare
Bah ok I fixed the 32-bit issue but the SIGSEGV went away there. I wasn't able to reproduce locally (not even with valgrind or address sanitizer!). |
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.
The point of using SmallVec is for performance, so here's some numbers for bench_parsing_debug_abbrev:
SmallVec: 9,989 ns/iter (+/- 468)
Attributes: 13,101 ns/iter (+/- 810)
Vec: 16,621 ns/iter (+/- 521)
So it's still an improvement over Vec at least, and maybe we can tune it more later. I think we should keep it private though.
a08d35c
to
1f57c66
Compare
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! Needs another rebase.
This commit removes `smallvec` as a dependency of the `read` feature of this crate. This is an extension of gimli-rs#494 to help make `gimli` easier to include into the standard library. The `smallvec` crate was only used in one location inside of `gimli`, in `Abbreviation` parsing. This was converted to a manual inline small-vector along with a new type `Attributes` to wrap this up. This also removes `smallvec` from the public API of `gimli` to allow future changes to the representation if necessary.
1f57c66
to
91a4446
Compare
Rebased! Sorry meant to comment that earlier. FWIW I don't want to make it seem like these dependency removals must happen to get the The primary purpose of this crate is a high-quality dwarf reader/writer, not meeting the weird requirements of libstd! It may be the case that addr2line doesn't need the pieces I'm removing these dependencies from so there may be alternatives or something like that to keep the deps but get the rough goal of "the Basically I want to reiterate that I don't want to place an undue maintenance burden on the gimli crates. The use case of |
The extra testing that will result from gimli being used in libstd will help improve improve its quality, so I think it's worth it. addr2line doesn't need the CFI support, which is where the unsafe code was added, so we potentially could make CFI a feature instead if it becomes unmanageable, but I think it's ok for now at least. Are there any plans to do unwinding using gimli too? Probably too far away to be worth worrying about now. |
Unwinding with libunwind has been reliable enough for now that I don't think there's any immediate need to switch to gimli for unwinding. It at least has been far more reliable than libbacktrace has been! I'd be totally down for a feature of the |
This commit removes
smallvec
as a dependency of theread
feature ofthis crate. This is an extension of #494 to help make
gimli
easier toinclude into the standard library. The
smallvec
crate was only used inone location inside of
gimli
, inAbbreviation
parsing. This wasconverted to a manual inline small-vector along with a new type
Attributes
to wrap this up. This also removessmallvec
from thepublic API of
gimli
to allow future changes to the representation ifnecessary.