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

StagingBelt: check for free chunks in the receiver as well as free_chunks. #2906

Merged
merged 3 commits into from
Jul 26, 2022

Conversation

kpreid
Copy link
Contributor

@kpreid kpreid commented Jul 21, 2022

Checklist

  • Run cargo clippy.
  • Run RUSTFLAGS=--cfg=web_sys_unstable_apis cargo clippy --target wasm32-unknown-unknown if applicable.
  • Add change to CHANGELOG.md.

Description
Previously, chunks would only be taken from the GPU callback receiver and put on free_chunks during finish(). So, there might be chunks that are actually available for use but wouldn't be used.

Now, we consult the receiver whenever we're about to consult the free_chunks, so the reuse of chunks will be as good as possible (given the application's uses of operations that trigger map_async callbacks).

Testing
StagingBelt has no automated tests, so I ran the examples/skybox and my own application that uses StagingBelt, and confirmed that there were no errors or noticeable negative performance consequences.

I don't have any proof that this is ever a performance improvement in practice; it just feels like low-hanging fruit.

…_chunks`.

Previously, chunks would only be taken from the GPU callback receiver
and put on `free_chunks` during `finish()`. So, there might be chunks
that are actually available for use but wouldn't be used.

Now, we consult the receiver whenever we're about to consult the
`free_chunks`, so the reuse of chunks will be as good as possible (given
the application's uses of operations that trigger `map_async` callbacks).
Copy link
Member

@cwfitzgerald cwfitzgerald left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dig it, just need a brief changelog entry :)

@cwfitzgerald cwfitzgerald enabled auto-merge (squash) July 26, 2022 14:50
@kpreid
Copy link
Contributor Author

kpreid commented Jul 26, 2022

Changelog entry added. I wasn't sure how to handle the conflict potential, and I know GitHub review reacts poorly to rebases or rewrites of existing commits, so I used a merge from master and commit. Please let me know if something else would go more smoothly.

@cwfitzgerald cwfitzgerald merged commit a420e45 into gfx-rs:master Jul 26, 2022
@kpreid kpreid deleted the beltopt branch July 26, 2022 15:07
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