Skip to content

feat: Out with flushing, in with batching #1684

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 38 commits into
base: main
Choose a base branch
from
Open

feat: Out with flushing, in with batching #1684

wants to merge 38 commits into from

Conversation

cieplypolar
Copy link
Collaborator

@cieplypolar cieplypolar commented Sep 10, 2025 •
edited
Loading

Changes:

  • draw, drawIndexed, dispatchWorkgroups flush immediately by default
  • write, writePartial, read, console.log always flush, even inside batch environment
  • added batch command on root['~unstable'] it allows to submit more than 1 command in command encoder
  • refactored some examples so that they use batch
  • added docs

Closes #1667.

TODO:

  • move flush() and commandEncoder() to root internals
  • add tests in root.test.ts
    • flush submits encoder, handle null, clears command encoder
    • commandEncoder() returns always not null, does not recreate without flush()
  • refactor render pipelines
  • add tests in renderPipeline.test.ts
  • refactor compute pipelines
  • add/change tests in computePipeline.test.ts
  • add batch() to root['~unstable']
  • add tests for batch()
  • refactor examples
  • docs
  • make performance callback flush at the end of the batch
  • add tests to read/write
  • edit docs
  • nested batch
  • integrate with console.log

@github-actions GitHub Actions
Copy link

github-actions bot commented Sep 10, 2025 •
edited
Loading

pkg.pr.new

packages
Ready to be installed by your favorite package manager ⬇️

https://pkg.pr.new/software-mansion/TypeGPU/typegpu@f5e7f3cf71be7c6a62c31ea495425c18d79e666d

https://pkg.pr.new/software-mansion/TypeGPU/@typegpu/noise@f5e7f3cf71be7c6a62c31ea495425c18d79e666d

https://pkg.pr.new/software-mansion/TypeGPU/unplugin-typegpu@f5e7f3cf71be7c6a62c31ea495425c18d79e666d

benchmark
view benchmark

commit
view commit

@cieplypolar cieplypolar marked this pull request as ready for review September 12, 2025 08:50
reczkok
reczkok reviewed Sep 12, 2025
View reviewed changes
Copy link
Contributor

@reczkok reczkok left a comment

Choose a reason for hiding this comment

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

I think pipelines with performacne callback should submit the callback functions to the batch to be run after instead of forcing the flush. This shouldn't affect the results of the performance callback at all and will simplify when we flush.

iwoplaza
iwoplaza requested changes Sep 12, 2025
View reviewed changes
reczkok
reczkok reviewed Sep 15, 2025
View reviewed changes
aleksanderkatan
aleksanderkatan reviewed Sep 18, 2025
View reviewed changes
Copy link
Contributor

@aleksanderkatan aleksanderkatan left a comment

Choose a reason for hiding this comment

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

Good job! 🚽

I leave some comments, mostly nits and stylistic preferences.

iwoplaza
iwoplaza reviewed Sep 26, 2025
View reviewed changes
packages/typegpu/src/core/pipeline/timeable.ts Outdated
return;
}
const result = await querySet.read();
querySet.read().then((result) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

We're reading the query set, but it's using its own command encoder, which means we will read the query set before resolving it. @reczkok What do you think?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I've investigated this, and you are right. I'll revisit the changes and add additional tests to avoid similar mistakes in the future.

cieplypolar
cieplypolar commented Sep 29, 2025
View reviewed changes
packages/typegpu/src/core/pipeline/timeable.ts
return;
}
const result = await querySet.read();
querySet.read().then(async (result) => {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

read handles flushing

cieplypolar
cieplypolar commented Sep 29, 2025
View reviewed changes
packages/typegpu/src/core/querySet/querySet.ts
this.count * BigUint64Array.BYTES_PER_ELEMENT,
);
this._group.device.queue.submit([commandEncoder.finish()]);
await this._group.device.queue.onSubmittedWorkDone();
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

we don't need to wait, because mapAsync waits for us for the busy buffer

aleksanderkatan
aleksanderkatan reviewed Sep 30, 2025
View reviewed changes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@iwoplaza iwoplaza iwoplaza requested changes

@aleksanderkatan aleksanderkatan aleksanderkatan left review comments

@reczkok reczkok reczkok left review comments

@lursz lursz Awaiting requested review from lursz

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

feat: Out with flushing, in with batching
4 participants
@cieplypolar @iwoplaza @aleksanderkatan @reczkok