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

Improve preview primitive performance #11942

Draft
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

Lain-B
Copy link
Collaborator

@Lain-B Lain-B commented Mar 10, 2025

Description

Minorly improves performance by reusing commonly used vertex buffers via a vertex buffer cache rather than creating/destroying vertex buffers every call. Also a minor improvement to performance by using a vertex buffer cache for drawing sprites as well, and also sizes sprites with a matrix rather than explicitly modifying vertices to the desired size unnecessarily (this should have been done a long time ago).

Motivation and Context

Due to #11917, I realized that the preview was recreating vertex buffers for every draw call, which is very unnecessary. I just decided to write a vertex buffer cache for it, and also ended up optimizing sprite drawing similarly in the process, reducing the need for modifying the sprite vertex buffer continually and instead utilizing simple caching for drawing sprites.

Note that this still has the impact of iterating through a vertex buffer array, so there is going to probably be a higher CPU impact, but it's all in a single line of memory and comparing the data. Still likely better than recreating vertex buffers every call. (I'm pretty sure at least.) The cache is designed to minimize iteration for most-used buffers, so it shouldn't be significant either way. The vertex buffer cache should only be like, a couple items max, so the change to vertex buffer should be minimal. But I'm willing to hear other ideas.

It's possible that this is just overall not a significant issue due to how few draw calls we have per frame, so if someone feels this should be closed that's fine, but I'm not entirely sure what the best way is to accurately compare.

How Has This Been Tested?

This affects all drawing so if there was something wrong it would probably affect everything if there were an issue, but it's because it affects everything that I'd like it tested as thoroughly as possible.

Note that I'm also not entirely sure how best to test this for perf improvement considering most of the impact is on the GPU. It'd be nice to know though.

Types of changes

  • Performance enhancement (non-breaking change which improves efficiency)

Checklist:

  • My code has been run through clang-format.
  • I have read the contributing document.
  • My code is not on the master branch.
  • The code has been tested.
  • All commit messages are properly formatted and commits squashed where appropriate.
  • I have included updates to all appropriate documentation.

Lain-B added 6 commits March 5, 2025 17:50
This recently was susceptible to integer overflows elsewhere, so set a
hard limit on the stripes.
A simple vertex buffer cache that allocates, reuses, and frees resources
as they're used and as they become unused.

Intended usage is with the preview widget of the frontend, but can also
be used with the sprite rendering.
These checks are redundant because the base function they all lead to
does the same check.
Minimize sprite vertex buffer updates by utilizing vertex buffer
caching. Should eliminate or at least minimize vertex buffer updates
when drawing sprites, and use matrix transforms for sprite sizes when
possible.
Before, due to vertices being unpredictable, vertex buffers were being
created/destroyed every draw call, which is very suboptimal. This change
minimizes vertex buffer updates via a vertex buffer cache.
@Lain-B
Copy link
Collaborator Author

Lain-B commented Mar 10, 2025

I just realized I need to add documentation for the vertex buffer cache.

@exeldro
Copy link
Contributor

exeldro commented Mar 11, 2025

I would set the cache size larger than 128, because a 1080p source with 1 px crop on each side needs more vertex buffers than that when selected. Using 256 cache size I get it when I make my preview as large as fits on my screen. Using 512 cache size I don't have that issue.
In DrawStripedLine a max stripe count of 1000 is added, so you could need 4000 (1000 for each side) vertex buffers.

When the max stripe count is reached only part of the line is drawn (first commit of this PR). It is the same kind of check as #11920 where that resorts to drawing a not striped line instead of part of a striped line.

@Lain-B
Copy link
Collaborator Author

Lain-B commented Mar 11, 2025

Yeah I'm starting to think that this idea was just not good. I'm going to think of better ways to improve the perf for these.

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