-
Notifications
You must be signed in to change notification settings - Fork 55
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
wrap Arc<Backtrace>
in Option, saving memory when backtraces are disabled
#252
base: main
Are you sure you want to change the base?
Conversation
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.
Looks like a nice small optimization. Thanks for your contribution! :D
I recall checking long ago that Regardless, this saves us some bytes in the |
a0fa1d3
to
94d82f1
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.
Looking at this PR I still wonder why the Backtrace
is wrapped in an Arc
; in e45e25a I wrote that that is to support "Clone
ing" the Backtrace
, but glancing at the implementation that is only because of mistakes in the ownership transfer in our fn allocate()
code and copies for AllocationReport
which should either not receive a Backtrace
at all, or borrow it temporarily while the caller can view the backtrace.
I don't have the time right now to investigate that further, feel free to do that yourself or leave the PR as is. |
This PR wraps all
Arc<Backtrace>
in Option, as to not have to allocate an Arc of a disabled Backtrace each time GPU memory is allocated with backtraces are disabled. This is probably a premature optimization, but it was so easy to implement I couldn't resist :DI only tested the vulkan implementation and visualizer on Linux, I hope CI catches issues for the other platforms.