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

[mtl] Refined passes #2288

Merged
merged 2 commits into from
Aug 3, 2018
Merged

[mtl] Refined passes #2288

merged 2 commits into from
Aug 3, 2018

Conversation

kvark
Copy link
Member

@kvark kvark commented Aug 2, 2018

Prevents rebinding of all the state and resources in case of consecutive compute dispatches.
Also, tries to make a list of commands for graphics pipeline setup and pass it as one big thing (instead of calling pre.issue for each individual one). This is piggy-backing on the issue_many function of the base commit here. Edit: removed this commit for causing a performance regression.

Changes the sink API to be more clear about how the passes are started/ended, it's now the same across compute/render and distinct from blits.

PR checklist:

  • make succeeds (on *nix)
  • make reftests succeeds
  • tested examples with the following backends: metal
  • rustfmt run on changed code

@kvark kvark requested a review from grovesNL August 2, 2018 02:17
@kvark
Copy link
Member Author

kvark commented Aug 2, 2018

hmm, I detect a performance regression here, investigating the cause now...

@kvark
Copy link
Member Author

kvark commented Aug 2, 2018

Good news - the regression is solely caused by the extra commit here, so I dropped it. It's an interesting piece that's not obvious to me: so building one gigantic chain of commands and recording them in one go is slower than calling pre.issue() for each individual one, contrary to my intuition. The news is good because it allows us to improve the performance expectations and revise all the places where we are currently using the long command chains.

@kvark kvark changed the title [mtl] sticky compute passes [mtl] Refined passes Aug 2, 2018
@@ -759,12 +759,29 @@ impl<'a> PreRender<'a> {
PreRender::Void => (),
}
}

fn issue_many<'b, I>(&mut self, commands: I)
Copy link
Contributor

Choose a reason for hiding this comment

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

Not really important for now, but it seems like we should be able to collapse these functions by being generic over the command type

{
{
let (mut pre, switch) = self.switch_compute();
assert!(switch);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this assertion guaranteed? It seems valid to have an encoder left open but call {fill|copy}_buffer, although I'm probably missing something here

Copy link
Member Author

Choose a reason for hiding this comment

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

good point! my assumption was wrong here

@kvark
Copy link
Member Author

kvark commented Aug 3, 2018

bors=grovesNL

@kvark
Copy link
Member Author

kvark commented Aug 3, 2018

bors r=grovesNL

bors bot added a commit that referenced this pull request Aug 3, 2018
2288: [mtl] Refined passes r=grovesNL a=kvark

Prevents rebinding of all the state and resources in case of consecutive compute dispatches.
~~Also, tries to make a list of commands for graphics pipeline setup and pass it as one big thing (instead of calling `pre.issue` for each individual one). This is piggy-backing on the `issue_many` function of the base commit here.~~ Edit: removed this commit for causing a performance regression.

Changes the sink API to be more clear about how the passes are started/ended, it's now the same across compute/render and distinct from blits.

PR checklist:
- [ ] `make` succeeds (on *nix)
- [x] `make reftests` succeeds
- [x] tested examples with the following backends: metal
- [ ] `rustfmt` run on changed code


Co-authored-by: Dzmitry Malyshau <kvarkus@gmail.com>
@kvark kvark mentioned this pull request Aug 3, 2018
4 tasks
@bors
Copy link
Contributor

bors bot commented Aug 3, 2018

@bors bors bot merged commit c080087 into gfx-rs:master Aug 3, 2018
@kvark kvark deleted the mtl-compute-pass branch August 3, 2018 20:50
bors bot added a commit that referenced this pull request Aug 4, 2018
2292: Metal command recording option r=grovesNL a=kvark

Based on #2288
Implements first bits of #2236

Allows the client to control the recording option without re-building gfx-rs.
Also hides the remote sink behind a feature. We can't CI-test it until SSheldon/rust-dispatch#10 is accepted and published.

PR checklist:
- [ ] `make` succeeds (on *nix)
- [ ] `make reftests` succeeds
- [ ] tested examples with the following backends:
- [ ] `rustfmt` run on changed code


Co-authored-by: Dzmitry Malyshau <kvarkus@gmail.com>
bors bot added a commit that referenced this pull request Aug 4, 2018
2292: Metal command recording option r=grovesNL a=kvark

Based on #2288
Implements first bits of #2236

Allows the client to control the recording option without re-building gfx-rs.
Also hides the remote sink behind a feature. We can't CI-test it until SSheldon/rust-dispatch#10 is accepted and published.

PR checklist:
- [ ] `make` succeeds (on *nix)
- [ ] `make reftests` succeeds
- [ ] tested examples with the following backends:
- [ ] `rustfmt` run on changed code


Co-authored-by: Dzmitry Malyshau <kvarkus@gmail.com>
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