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

Make backtrace dependency optional #183

Closed

Conversation

stefnotch
Copy link

I was going through my dependencies, trying to slim down the dependency tree. I saw that backtrace depends on cc, which might be expensive. So I tried to put backtrace inside a feature that can be turned off.

The results aren't all that impressive though

  • I got to shave off around 5 crates for a full recompile. Might translate into 1 second of CI time.
  • Maybe a 10 ms reduction in incremental compile times, if anything at all.

@MarijnS95
Copy link
Member

Fyi I've been working on removing this crate altogether in favour of stabilized/builtin support since Rust 1.65. It was tricky back then because of ownership and clones, but might be more manageable since recent changes:

https://github.com/Traverse-Research/gpu-allocator/compare/std-backtrace

@stefnotch
Copy link
Author

stefnotch commented Oct 12, 2023

@MarijnS95 Oh that's so cool! Any way I could help?

There doesn't seem to be a test nor example for the log_stack_traces or store_stack_traces, is that right? As in, I'd best

  1. Add an example for that
  2. Switch to the std backtrace
  3. Verify that the example still works
  4. Make a nice PR

@stefnotch
Copy link
Author

stefnotch commented Oct 12, 2023

@MarijnS95 Okay, I took a look at it. From what I can tell, there are two options.

  1. Option<Arc<Backtrace>>
  2. Lifetimes, like turning the DedicatedBlockAllocator into a DedicatedBlockAllocator<'a>. I think this one is much harder, since it'd infect so much code with lifetimes.

Do you have any preferences?

Also, I wish there were a visualizer example. But that's a future request, I suppose.

@stefnotch stefnotch mentioned this pull request Oct 12, 2023
@MarijnS95
Copy link
Member

@MarijnS95 Oh that's so cool! Any way I could help?

No, I'll probably revise and submit it later 😉

@stefnotch
Copy link
Author

@MarijnS95 Sounds good. I did make a PR for it, but I'll leave it up to you to figure out the optimal solution :)

@MarijnS95
Copy link
Member

@stefnotch as promised, a PR from that branch is now open at #186. In particular I've addressed the TODOs left in that branch such as using proper Display instead of Debug prints, and making practical use of Backtrace::disabled() 🎉

@MarijnS95
Copy link
Member

Also, I wish there were a visualizer example. But that's a future request, I suppose.

On this note, these were unfortunately all removed: #176

In part because we identified a problem way back in the beginning where Vulkan and Dx12 visualizers were basically a massive (verbose, hard to maintain) copy-paste of each-other, rather than going through a cross-compatible interim data-structure saving us all that overhead. Contributions in this area are welcome I think, but let's hear from @tosti007 whether any of this is already in-progress on a branch somewhere.

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.

2 participants