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] new autorelease #2267

Merged
merged 1 commit into from
Jul 24, 2018
Merged

[mtl] new autorelease #2267

merged 1 commit into from
Jul 24, 2018

Conversation

kvark
Copy link
Member

@kvark kvark commented Jul 24, 2018

Fixes the time wasted on setting up temporary autorelease pools, mostly. Although the resulting FPS doesn't appear to be affected. At least we no longer depend on core-foundation :)
cc @jrmuizel (and thanks!) for the way it looks now

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 July 24, 2018 13:24
Copy link
Contributor

@grovesNL grovesNL left a comment

Choose a reason for hiding this comment

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

Looks great! Much cleaner IMO

@@ -721,7 +722,6 @@ impl Journal {
}

fn record(&self, command_buf: &metal::CommandBufferRef) {
let _ap = AutoreleasePool::new(); // for encoder creation
Copy link
Contributor

Choose a reason for hiding this comment

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

This removal seems ok but maybe we should indicate that this is handled by https://github.com/gfx-rs/gfx/pull/2267/files#diff-abc29e1de3205709a964ff5e3b0e6333R1494 instead

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't know the best way for us to enforce the autorelease pools, tbh. Just trying to put comments doesn't protect us against a breakage during refactor, or from using a method accidentally that requires a pool but we don't have one...

@kvark
Copy link
Member Author

kvark commented Jul 24, 2018

Let's keep thinking on how to enforce this nicer...
bors r=grovesNL

bors bot added a commit that referenced this pull request Jul 24, 2018
2267: [mtl] new autorelease r=grovesNL a=kvark

Fixes the time wasted on setting up temporary autorelease pools, mostly. Although the resulting FPS doesn't appear to be affected. At least we no longer depend on core-foundation :)
cc @jrmuizel (and thanks!) for the way it looks now

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


Co-authored-by: Dzmitry Malyshau <kvarkus@gmail.com>
@bors
Copy link
Contributor

bors bot commented Jul 24, 2018

@bors bors bot merged commit d8a6e39 into gfx-rs:master Jul 24, 2018
@kvark kvark mentioned this pull request Jul 24, 2018
22 tasks
@kvark kvark deleted the autorelease branch July 24, 2018 16:20
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.

None yet

2 participants