GPU: Optimize GLStorageBuf::read performance #117521

Miguel Pozo · 2024-01-25T20:29:27+01:00

Miguel Pozo commented

2024-01-25 20:29:27 +01:00

Add a separate persistent mapped buffer where the main SSBO can be copied,
so its contents can be read from the CPU without stalling the GPU.

Add a separate persistent mapped buffer where the main SSBO can be copied, so its contents can be read from the CPU without stalling the GPU.

Miguel Pozo added the

Module

EEVEE & Viewport

label 2024-01-25 20:29:27 +01:00

Miguel Pozo added 2 commits 2024-01-25 20:29:40 +01:00

9f46edd1c0 Use a separate persistent memory buffer for SSBO reads

91d13f47aa Match Metal behavior

Miguel Pozo requested review from Jeroen Bakker 2024-01-25 20:30:08 +01:00

Miguel Pozo requested review from Clément Foucault 2024-01-25 20:30:08 +01:00

Miguel Pozo referenced this pull request

2024-01-25 20:31:17 +01:00

EEVEE-Next: Optimize ShadowStatistics read #117302

Clément Foucault requested changes 2024-01-26 01:52:06 +01:00

Clément Foucault left a comment

For something like this, I would really like to see a test being written to make sure it is working on all platforms.

Miguel Pozo added 1 commit 2024-01-26 16:53:49 +01:00

de9c950cb8 Match expected behavior from test units

Miguel Pozo commented

2024-01-26 17:18:38 +01:00

Do you have any specific test in mind?
There are already SSBO tests and plenty of tests that rely on ssbo reads.

In fact, I've noticed I broke several tests because they expect to be able to just call read without a previous flush call.
I've also seen there's a GPU_BARRIER_BUFFER_UPDATE that's used in some places, but I'm not sure how much sense it makes to leak something like that into the draw code.
In this case it needs a different type of barrier (there's a commented out GPU_BARRIER_CLIENT_MAPPED_BUFFER, but again, I think it would be best to leave this as an implementation detail) and the barrier itself doesn't do anything useful since this implementation needs an internal buffer copy too.

I've updated the PR so the tests work again, but IMO it would make more sense to do something like this:

Get rid of GPU_BARRIER_BUFFER_UPDATE and GPU_BARRIER_CLIENT_MAPPED_BUFFER, and use async_flush_to_host instead where needed.
Add an async parameter to read (defaults to false) so:
- If async is false and there's a pending async_flush_to_host it waits for its fence.
- If async is false and there's not a pending async_flush_to_host, it calls it in-place and waits for its fence.
- If async is true, it's assumed that the user has called async_flush_to_host before and the fence is ignored.

Do you have any specific test in mind? There are already SSBO tests and plenty of tests that rely on ssbo reads. In fact, I've noticed I broke several tests because they expect to be able to just call read without a previous flush call. I've also seen there's a `GPU_BARRIER_BUFFER_UPDATE` that's used in some places, but I'm not sure how much sense it makes to leak something like that into the draw code. In this case it needs a different type of barrier (there's a commented out `GPU_BARRIER_CLIENT_MAPPED_BUFFER`, but again, I think it would be best to leave this as an implementation detail) and the barrier itself doesn't do anything useful since this implementation needs an internal buffer copy too. I've updated the PR so the tests work again, but IMO it would make more sense to do something like this: - Get rid of `GPU_BARRIER_BUFFER_UPDATE` and `GPU_BARRIER_CLIENT_MAPPED_BUFFER`, and use `async_flush_to_host` instead where needed. - Add an `async` parameter to `read` (defaults to false) so: * If `async` is false and there's a pending `async_flush_to_host` it waits for its fence. * If `async` is false and there's not a pending `async_flush_to_host`, it calls it in-place and waits for its fence. * If `async` is true, it's assumed that the user has called `async_flush_to_host` before and the fence is ignored.

Miguel Pozo referenced this pull request

2024-01-26 20:51:24 +01:00

EEVEE-Next: Performance Analysis #117246

Miguel Pozo added 1 commit 2024-02-08 20:21:01 +01:00

d10f3bd708 Clarify GPU_storagebuf_read comment

Miguel Pozo commented

2024-02-08 20:24:47 +01:00

In the end, I just updated the GPU_storagebuf_read comment so it better describes the currently expected behavior.

In the end, I just updated the `GPU_storagebuf_read` comment so it better describes the currently expected behavior.

Clément Foucault reviewed 2024-02-09 13:49:16 +01:00

source/blender/gpu/opengl/gl_storage_buffer.cc Outdated

						
				@ -184,0 +236,4 @@

				  glDeleteSync(read_fence_);

				  read_fence_ = 0;

				  memmove(data, persistent_ptr_, size_in_bytes_);

Clément Foucault commented

2024-02-09 13:49:12 +01:00

Why memmove instead of memcpy?

Why `memmove` instead of `memcpy`?

pragma37 marked this conversation as resolved

Clément Foucault approved these changes 2024-02-09 14:38:51 +01:00

Clément Foucault left a comment

Tests passes on my machine. Given you update the PR description, this is good to be merged.

Clément Foucault reviewed 2024-02-09 14:39:20 +01:00

source/blender/gpu/opengl/gl_storage_buffer.cc Outdated

						
				@ -7,6 +7,7 @@

				 */

				#include "BLI_string.h"

				#include "BLI_time.h"

Clément Foucault commented

2024-02-09 14:39:20 +01:00

Uneeded include.

pragma37 marked this conversation as resolved

Miguel Pozo added 2 commits 2024-02-09 16:09:48 +01:00

86dec4619c Remove unused include

217cae32d0 Replace memmove with memcpy

Miguel Pozo merged commit 98231ea880 into main

2024-02-09 16:11:44 +01:00

Miguel Pozo referenced this issue from a commit

2024-02-09 16:11:45 +01:00

GPU: Optimize GLStorageBuf::read performance

Miguel Pozo deleted branch pull-eevee-optimize-shadow-statistics-2

2024-02-09 16:11:47 +01:00

Sign in to join this conversation.

No reviewers

No Label

Download

What's New

Blender Studio

Manual

Developers Blog

Documentation

Benchmark

Blender Conference

Development Fund

One-time Donations

GPU: Optimize GLStorageBuf::read performance #117521