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

WGPU leaks the implicit PLL of pipeline layouts #5029

Closed
DevJac opened this issue Jan 9, 2024 · 6 comments · Fixed by #5971
Closed

WGPU leaks the implicit PLL of pipeline layouts #5029

DevJac opened this issue Jan 9, 2024 · 6 comments · Fixed by #5971
Assignees
Labels
type: bug Something isn't working

Comments

@DevJac
Copy link
Contributor

DevJac commented Jan 9, 2024

This 300 line graphics application leaks memory: https://github.com/DevJac/life_sim/blob/36a55a1a4762bb140d1fed2f04011e7f3d030203/src/main.rs

The code is in a single file, which I've also attached to this issue if needed for future reference.
main.txt

My render code is unusual because it recreates the pipeline each frame, I'm learning so I've been focused on forward progress rather than optimal code.

After 10,123 frames rendered, I get the following report from Instance:

Instance report: GlobalReport { surfaces: StorageReport { num_occupied: 1, num_vacant: 0, num_error: 0, element_size: 568 }, vulkan: Some(HubReport { adapters: StorageReport { num_occupied: 1, num_vacant: 0, num_error: 0, element_size: 1880 }, devices: StorageReport { num_occupied: 1, num_vacant: 0, num_error: 0, element_size: 12008 }, pipeline_layouts: StorageReport { num_occupied: 10123, num_vacant: 0, num_error: 0, element_size: 176 }, shader_modules: StorageReport { num_occupied: 0, num_vacant: 1, num_error: 0, element_size: 744 }, bind_group_layouts: StorageReport { num_occupied: 1, num_vacant: 0, num_error: 80983, element_size: 240 }, bind_groups: StorageReport { num_occupied: 0, num_vacant: 1, num_error: 0, element_size: 312 }, command_buffers: StorageReport { num_occupied: 0, num_vacant: 1, num_error: 0, element_size: 1416 }, render_bundles: StorageReport { num_occupied: 0, num_vacant: 0, num_error: 0, element_size: 904 }, render_pipelines: StorageReport { num_occupied: 0, num_vacant: 1, num_error: 0, element_size: 512 }, compute_pipelines: StorageReport { num_occupied: 0, num_vacant: 0, num_error: 0, element_size: 256 }, query_sets: StorageReport { num_occupied: 0, num_vacant: 0, num_error: 0, element_size: 48 }, buffers: StorageReport { num_occupied: 2, num_vacant: 2, num_error: 0, element_size: 304 }, textures: StorageReport { num_occupied: 0, num_vacant: 2, num_error: 0, element_size: 784 }, texture_views: StorageReport { num_occupied: 0, num_vacant: 2, num_error: 0, element_size: 208 }, samplers: StorageReport { num_occupied: 0, num_vacant: 0, num_error: 0, element_size: 48 } }), gl: Some(HubReport { adapters: StorageReport { num_occupied: 0, num_vacant: 0, num_error: 0, element_size: 272 }, devices: StorageReport { num_occupied: 0, num_vacant: 0, num_error: 0, element_size: 5544 }, pipeline_layouts: StorageReport { num_occupied: 0, num_vacant: 0, num_error: 0, element_size: 200 }, shader_modules: StorageReport { num_occupied: 0, num_vacant: 0, num_error: 0, element_size: 768 }, bind_group_layouts: StorageReport { num_occupied: 0, num_vacant: 0, num_error: 0, element_size: 152 }, bind_groups: StorageReport { num_occupied: 0, num_vacant: 0, num_error: 0, element_size: 248 }, command_buffers: StorageReport { num_occupied: 0, num_vacant: 0, num_error: 0, element_size: 4168 }, render_bundles: StorageReport { num_occupied: 0, num_vacant: 0, num_error: 0, element_size: 904 }, render_pipelines: StorageReport { num_occupied: 0, num_vacant: 0, num_error: 0, element_size: 672 }, compute_pipelines: StorageReport { num_occupied: 0, num_vacant: 0, num_error: 0, element_size: 256 }, query_sets: StorageReport { num_occupied: 0, num_vacant: 0, num_error: 0, element_size: 64 }, buffers: StorageReport { num_occupied: 0, num_vacant: 0, num_error: 0, element_size: 192 }, textures: StorageReport { num_occupied: 0, num_vacant: 0, num_error: 0, element_size: 672 }, texture_views: StorageReport { num_occupied: 0, num_vacant: 0, num_error: 0, element_size: 184 }, samplers: StorageReport { num_occupied: 0, num_vacant: 0, num_error: 0, element_size: 40 } }) }

or formatted:

GlobalReport {
	     surfaces: StorageReport { num_occupied: 1, num_vacant: 0, num_error: 0, element_size: 568 },
	     vulkan: Some(HubReport {
	     	     adapters: StorageReport { num_occupied: 1, num_vacant: 0, num_error: 0, element_size: 1880 },
		     devices: StorageReport { num_occupied: 1, num_vacant: 0, num_error: 0, element_size: 12008 },
		     pipeline_layouts: StorageReport { num_occupied: 10123, num_vacant: 0, num_error: 0, element_size: 176 },
		     shader_modules: StorageReport { num_occupied: 0, num_vacant: 1, num_error: 0, element_size: 744 },
		     bind_group_layouts: StorageReport { num_occupied: 1, num_vacant: 0, num_error: 80983, element_size: 240 },
		     bind_groups: StorageReport { num_occupied: 0, num_vacant: 1, num_error: 0, element_size: 312 },
		     command_buffers: StorageReport { num_occupied: 0, num_vacant: 1, num_error: 0, element_size: 1416 },
		     render_bundles: StorageReport { num_occupied: 0, num_vacant: 0, num_error: 0, element_size: 904 },
		     render_pipelines: StorageReport { num_occupied: 0, num_vacant: 1, num_error: 0, element_size: 512 },
		     compute_pipelines: StorageReport { num_occupied: 0, num_vacant: 0, num_error: 0, element_size: 256 },
		     query_sets: StorageReport { num_occupied: 0, num_vacant: 0, num_error: 0, element_size: 48 },
		     buffers: StorageReport { num_occupied: 2, num_vacant: 2, num_error: 0, element_size: 304 },
		     textures: StorageReport { num_occupied: 0, num_vacant: 2, num_error: 0, element_size: 784 },
		     texture_views: StorageReport { num_occupied: 0, num_vacant: 2, num_error: 0, element_size: 208 },
		     samplers: StorageReport { num_occupied: 0, num_vacant: 0, num_error: 0, element_size: 48 } }),
	     gl: Some(HubReport {
	     	     adapters: StorageReport { num_occupied: 0, num_vacant: 0, num_error: 0, element_size: 272 },
		     devices: StorageReport { num_occupied: 0, num_vacant: 0, num_error: 0, element_size: 5544 },
		     pipeline_layouts: StorageReport { num_occupied: 0, num_vacant: 0, num_error: 0, element_size: 200 },
		     shader_modules: StorageReport { num_occupied: 0, num_vacant: 0, num_error: 0, element_size: 768 },
		     bind_group_layouts: StorageReport { num_occupied: 0, num_vacant: 0, num_error: 0, element_size: 152 },
		     bind_groups: StorageReport { num_occupied: 0, num_vacant: 0, num_error: 0, element_size: 248 },
		     command_buffers: StorageReport { num_occupied: 0, num_vacant: 0, num_error: 0, element_size: 4168 },
		     render_bundles: StorageReport { num_occupied: 0, num_vacant: 0, num_error: 0, element_size: 904 },
		     render_pipelines: StorageReport { num_occupied: 0, num_vacant: 0, num_error: 0, element_size: 672 },
		     compute_pipelines: StorageReport { num_occupied: 0, num_vacant: 0, num_error: 0, element_size: 256 },
		     query_sets: StorageReport { num_occupied: 0, num_vacant: 0, num_error: 0, element_size: 64 },
		     buffers: StorageReport { num_occupied: 0, num_vacant: 0, num_error: 0, element_size: 192 },
		     textures: StorageReport { num_occupied: 0, num_vacant: 0, num_error: 0, element_size: 672 },
		     texture_views: StorageReport { num_occupied: 0, num_vacant: 0, num_error: 0, element_size: 184 },
		     samplers: StorageReport { num_occupied: 0, num_vacant: 0, num_error: 0, element_size: 40 } })
}

Notice the number of pipeline_layouts equals the number of frames rendered. This appears to be the memory leak. The counts of all other objects are low.

@DevJac
Copy link
Contributor Author

DevJac commented Jan 9, 2024

In Discord cwfitzgerald had the following to say:

Buttons
 — 
Today at 3:06 PM
@cwfitzgerald I'm still having that memory leak we discussed yesterday. I've simplified my code to a single file: https://github.com/DevJac/life_sim/blob/simplify_and_debug_mem_leak/src/main.rs

I'm wondering if it's related to https://github.com/gfx-rs/wgpu/issues/4073 ?

This leaks about 200 megabytes per second with a release build. I've seen the leak happening on both my linux and mac computer. I don't think it's actually leaked memory, I think WGPU is just holding onto the memory for too long, because when I run the app with valgrind it doesn't detect any significant leaks, so the memory does get cleared at the end of the program it seems. Should I make a github  issue?
cwfitzgerald
 — 
Today at 3:07 PM
could be! Yes please file and mention that issue, as they do smell related
both examples are creating many pipelines, which is generally abnormal
@Buttonsalso, after it's leaked for a bit, could you call this wacky function https://docs.rs/wgpu/latest/wgpu/struct.Instance.html#method.generate_report and debug print it and put that result.. .somewhere
[Instance in wgpu - Rust](https://docs.rs/wgpu/latest/wgpu/struct.Instance.html)
Context for all other wgpu objects. Instance of wgpu.
Buttons
 — 
Today at 3:13 PM
You just want debug string of that GlobalReport? 
cwfitzgerald
 — 
Today at 3:14 PM
yeah
Buttons
 — 
Today at 3:16 PM
after 10_123 iterations report includes: pipeline_layouts: StorageReport { num_occupied: 10123, num_vacant: 0, num_error: 0, element_size: 176 } 
I don't define my own pipeline layout, but use the one inferred by the pipeline
cwfitzgerald
 — 
Today at 3:20 PM
hmm that's definitely not right
ahhh implicit PLL
is compute_pipelines still a low amount?
Buttons
 — 
Today at 3:21 PM
yes, all other num_occupieds are low
cwfitzgerald
 — 
Today at 3:21 PM
welp there's the problem then, we're leaking the implicit PLL
the PLL code is a bit of a disaster, but shouldn't be too terrible to fix
(editors note: it was a much worse disaster pre-arcanization, so things are getting better!) 
Buttons
 — 
Today at 3:22 PM
i'm making a github issue

@waynr
Copy link
Contributor

waynr commented Jan 10, 2024

@DevJac it looks like you recreate the render pipeline every time draw_lines is called. In that method all you should have to do is write to existing Buffer instances to update the content available for the shader to process.

I don't think it (recreating the pipeline every time) should be necessary because in my wgpu application I hold on to a reference to the RenderPipeline and various Buffer instances and reuse those on every frame. I was careful to set it up this way explicitly because i have to think about when i should be recompiling shader files (due to the nature of my app) at runtime but I think I would want to avoid recompiling the shader on every draw call even if my shader were statically compiled into the program as yours is (see https://github.com/DevJac/life_sim/blob/36a55a1a4762bb140d1fed2f04011e7f3d030203/src/main.rs#L127C59-L127C71).

Would an implicit PLL imply an implicit BindGroupLayout? I see that the example code does create and use BindGoups so in addition to not recompiling the shader on ever frame it might be helpful to explicitly create a PLL referencing the BindGroup layouts.

There is a wgpu tutorial that is careful about managing state and definitely keeps its RenderPipeline in its State struct (which would correspond to your Renderer struct).

Also, I've found that wgpu makes good use of DEBUG and TRACE logs using the tracing library (TRACE-level wgpu logs could be helpful with issues like this).

Also, to be clear:

  • I know very little about wgpu and even less about your app so take my suggestion with a grain of salt :)
  • I do agree that implicitly-created pipeline layouts being leaked sounds bad (just so it's clear nothing i say is a reason to close this ticket)

@DevJac
Copy link
Contributor Author

DevJac commented Jan 10, 2024

Yes, my code is not optimized. Also, this code is what's left over after simplifying and removing almost everything else.

I wanted to focus on writing simple and clean Rust code first, then profile, then optimize where it makes sense. You're right, it is not optimal to recreate the pipeline and recompile the shaders and everything else I'm doing every single frame, but I'm still getting 1000 FPS currently, so it's fast enough. My plan was to optimize when needed, and the optimization will be easy since the code is simple.

Whatever the case, this code is not optimal, but it's not doing anything wrong, and I believe WGPU should not be leaking. I'm aware that if I need a workaround, I can avoid recreating the pipeline (etc) over and over and that will work around the memory leak. Ideally the memory leak would be fixed one way or another.

@cwfitzgerald
Copy link
Member

cwfitzgerald commented Jan 10, 2024

Yup, this leak shouldn't be happening and we should fix it, no matter what the code is actually doing

@teoxoy teoxoy added the type: bug Something isn't working label Jan 22, 2024
@jimblandy
Copy link
Member

I wonder if this is #3572.

@teoxoy
Copy link
Member

teoxoy commented Jul 17, 2024

This is caused by the Hub holding strong references to the derived PL and BGLs. wgpu doesn't drop those when it drops the pipeline; but since we don't depend on IDs internally anymore I think we can just stop placing those derived resources in the Hub for users that don't provide IDs (like wgpu).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug Something isn't working
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

5 participants