Traverse-Research / gpu-allocator

🦀 GPU memory allocator for Vulkan, DirectX 12 and Metal. Written in pure Rust
https://traverse.nl/
Apache License 2.0
380 stars 50 forks source link

Add a public API to expose allocation reports #226

Closed nical closed 4 months ago

nical commented 5 months ago

This PR adds a public API that exposes enough information for users to build something like the block visualizer without the egui integration. It also adds an offset field in the allocation report.

I would like to integrate (and expose in some form) this in wgpu to debug memory leaks.

The report could expose more information (for example properties of the memory and stack traces) and I haven't added any documentation yet, but it's a good basis for discussing whether the project is OK with exposing something like this and whether the approach is good.

Jasper-Bekkers commented 5 months ago

Tracks towards #203 so very welcome. Also indirectly helps with #59

nical commented 5 months ago

I made a few notable changes:

nical commented 5 months ago

I added metal and made a few other adjustments from the review comments. I still haven't added information about the memory blocks to the report. To do that I need to spend a bit of time looking at each backend, finding the common denominators and storing the right info in the right places for the report to pick up. It's not much but because I am currently juggling a lot of tasks on different projects I'm having some difficulty finding the time to wrap my head around that bit.

Would you be OK with landing the current feature set? Should I bump the patch version in this PR?

MarijnS95 commented 5 months ago

Should I bump the patch version in this PR?

I'm sure wgpu has special release processes that don't like arbitrary PRs bumping the version either :wink:


We currently have a CI failure related to the whole metal-rs "debacle" that might be blocking the merge of this PR. We'll have to resolve that first before this can be merged.

nical commented 5 months ago

I'm sure wgpu has special release processes that don't like arbitrary PRs bumping the version either 😉

I don't think there's a specific rule about that but since wgpu's release process is fair bit more involved than most crates, it indeed tends to be done in isolation.

We currently have a CI failure related to the whole metal-rs "debacle"

I wasn't aware of this. I'm not involved with the metal stuff and reading through https://github.com/Traverse-Research/gpu-allocator/issues/224 and linked issues it looks like the interesting stuff might be happening upstream of the metal-rs crate but let me know if I can help by following up with other folks in the gfx-rs org.

MarijnS95 commented 5 months ago

I don't think there's a specific rule about that but since wgpu's release process is fair bit more involved than most crates, it indeed tends to be done in isolation.

So is ours, and I think for every crate it's a much nicer policy to have an explicit "Release XXX" commit in their history rather than doing it within a feature PR.

MarijnS95 commented 4 months ago

@nical would you mind to sign your commits?

MarijnS95 commented 4 months ago

@nical JFYI, despite being subscribed to this repo and PR I don't receive any notifications for your pushes, and just happened to see the update at the right moment by accident. Please ping me once I have to approve the CI again, since GitHub delegates their anti-CI-abuse measures to repository owners and maintainers :sweat_smile:

nical commented 4 months ago

Good to know, @MarijnS95. I just pushed another commit to fix the fmt issue