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.

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

[Merged by Bors] - Skinned extraction speedup #4428

Closed
wants to merge 4 commits into from
Closed

[Merged by Bors] - Skinned extraction speedup #4428

wants to merge 4 commits into from

Conversation

mockersf
Copy link
Member

@mockersf mockersf commented Apr 5, 2022 •
edited
Loading

Objective

Solution

  • Use Vec instead of a SmallVec
  • Don't use an temporary variable
  • Compute the affine matrix as an Affine3A instead
  • Remove the temp vec
mean
base 15.17ms
vec 9.31ms
no temp variable 11.31ms
removing the temp vector 8.43ms
affine 13.21ms
all together 7.23ms

@github-actions github-actions bot added the S-Needs-Triage This issue needs to be labelled label Apr 5, 2022
@superdump
Copy link
Contributor

I’d be interested to make. GlobalTransform just an Affine3A internally and not a separate representation at all, and see what impact that has on relevant systems… :)

@superdump superdump added C-Performance A change motivated by improving speed, memory usage or compile times A-Animation Make things move and change over time and removed S-Needs-Triage This issue needs to be labelled labels Apr 5, 2022
@mockersf
Copy link
Member Author

mockersf commented Apr 5, 2022

yup that would probably be faster, but also a much bigger bite to take...

@superdump
Copy link
Contributor

Also, the title is incorrect. An almost 48% reduction in execution time is enormous.

@james7132
Copy link
Member

james7132 commented Apr 5, 2022 •
edited
Loading

Copying my comments from Discord:

[2:12 PM] james7132: the temp buffer is only added in the case of a failure to fetch.
[2:12 PM] james7132: if we can either remove the failure case
[2:12 PM] james7132: or find another way to fail without breaking the state of the total joint buffer, we could probably save quite a bit of time

If we can avoid this double buffered copy, I think we could save quite a bit more time too. Can you test saving the length before appending, directly push items to the total joint buffer, and truncate-and-return back to the start length if there is a failure to fetch? This should let us handle the failure case without needing a temporary buffer.

@mockersf @james7132
remove the temp vec
f192fcc 
Co-Authored-By: James Liu <3137680+james7132@users.noreply.github.com>
@mockersf
Copy link
Member Author

mockersf commented Apr 5, 2022

removed the need of the temp vec for 1ms extra gain 🎉

@mockersf mockersf changed the title Skinned extraction small speedup Skinned extraction speedup Apr 5, 2022
cart
cart approved these changes Apr 6, 2022
View reviewed changes
Copy link
Member

@cart cart left a comment

Choose a reason for hiding this comment

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

These are some tasty wins. Well done!

@cart
Copy link
Member

cart commented Apr 6, 2022

bors r+

bors bot pushed a commit that referenced this pull request Apr 6, 2022
@mockersf
Skinned extraction speedup (#4428)
9ef5f60 
# Objective

- While animating 501 https://github.com/KhronosGroup/glTF-Sample-Models/tree/master/2.0/BrainStem, I noticed things were getting a little slow
- Looking in tracy, the system `extract_skinned_meshes` is taking a lot of time, with a mean duration of 15.17ms

## Solution

- ~~Use `Vec` instead of a `SmallVec`~~
- ~~Don't use an temporary variable~~
- Compute the affine matrix as an `Affine3A` instead
- Remove the `temp` vec

| |mean|
|---|---|
|base|15.17ms|
|~~vec~~|~~9.31ms~~|
|~~no temp variable~~|~~11.31ms~~|
|removing the temp vector|8.43ms|
|affine|13.21ms|
|all together|7.23ms|
@bors
Copy link
Contributor

bors bot commented Apr 6, 2022

This PR was included in a batch that timed out, it will be automatically retried

bors bot pushed a commit that referenced this pull request Apr 6, 2022
@mockersf
Skinned extraction speedup (#4428)
29f86e3 
# Objective

- While animating 501 https://github.com/KhronosGroup/glTF-Sample-Models/tree/master/2.0/BrainStem, I noticed things were getting a little slow
- Looking in tracy, the system `extract_skinned_meshes` is taking a lot of time, with a mean duration of 15.17ms

## Solution

- ~~Use `Vec` instead of a `SmallVec`~~
- ~~Don't use an temporary variable~~
- Compute the affine matrix as an `Affine3A` instead
- Remove the `temp` vec

| |mean|
|---|---|
|base|15.17ms|
|~~vec~~|~~9.31ms~~|
|~~no temp variable~~|~~11.31ms~~|
|removing the temp vector|8.43ms|
|affine|13.21ms|
|all together|7.23ms|
@bors
Copy link
Contributor

bors bot commented Apr 6, 2022

This PR was included in a batch that timed out, it will be automatically retried

bors bot pushed a commit that referenced this pull request Apr 7, 2022
@mockersf
Skinned extraction speedup (#4428)
34b4fb6 
# Objective

- While animating 501 https://github.com/KhronosGroup/glTF-Sample-Models/tree/master/2.0/BrainStem, I noticed things were getting a little slow
- Looking in tracy, the system `extract_skinned_meshes` is taking a lot of time, with a mean duration of 15.17ms

## Solution

- ~~Use `Vec` instead of a `SmallVec`~~
- ~~Don't use an temporary variable~~
- Compute the affine matrix as an `Affine3A` instead
- Remove the `temp` vec

| |mean|
|---|---|
|base|15.17ms|
|~~vec~~|~~9.31ms~~|
|~~no temp variable~~|~~11.31ms~~|
|removing the temp vector|8.43ms|
|affine|13.21ms|
|all together|7.23ms|
@bors
Copy link
Contributor

bors bot commented Apr 7, 2022

Timed out.

@mockersf
Copy link
Member Author

mockersf commented Apr 7, 2022

bors r+

bors bot pushed a commit that referenced this pull request Apr 7, 2022
@mockersf
Skinned extraction speedup (#4428)
9d54f33 
# Objective

- While animating 501 https://github.com/KhronosGroup/glTF-Sample-Models/tree/master/2.0/BrainStem, I noticed things were getting a little slow
- Looking in tracy, the system `extract_skinned_meshes` is taking a lot of time, with a mean duration of 15.17ms

## Solution

- ~~Use `Vec` instead of a `SmallVec`~~
- ~~Don't use an temporary variable~~
- Compute the affine matrix as an `Affine3A` instead
- Remove the `temp` vec

| |mean|
|---|---|
|base|15.17ms|
|~~vec~~|~~9.31ms~~|
|~~no temp variable~~|~~11.31ms~~|
|removing the temp vector|8.43ms|
|affine|13.21ms|
|all together|7.23ms|
@bors
Copy link
Contributor

bors bot commented Apr 7, 2022

Pull request successfully merged into main.

Build succeeded:

@bors bors bot changed the title Skinned extraction speedup [Merged by Bors] - Skinned extraction speedup Apr 7, 2022
@bors bors bot closed this Apr 7, 2022
aevyrie pushed a commit to aevyrie/bevy that referenced this pull request Jun 7, 2022
@mockersf @aevyrie
Skinned extraction speedup (bevyengine#4428)
df8e2b5 
# Objective

- While animating 501 https://github.com/KhronosGroup/glTF-Sample-Models/tree/master/2.0/BrainStem, I noticed things were getting a little slow
- Looking in tracy, the system `extract_skinned_meshes` is taking a lot of time, with a mean duration of 15.17ms

## Solution

- ~~Use `Vec` instead of a `SmallVec`~~
- ~~Don't use an temporary variable~~
- Compute the affine matrix as an `Affine3A` instead
- Remove the `temp` vec

| |mean|
|---|---|
|base|15.17ms|
|~~vec~~|~~9.31ms~~|
|~~no temp variable~~|~~11.31ms~~|
|removing the temp vector|8.43ms|
|affine|13.21ms|
|all together|7.23ms|
ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023
@mockersf @ItsDoot
Skinned extraction speedup (bevyengine#4428)
098471f 
# Objective

- While animating 501 https://github.com/KhronosGroup/glTF-Sample-Models/tree/master/2.0/BrainStem, I noticed things were getting a little slow
- Looking in tracy, the system `extract_skinned_meshes` is taking a lot of time, with a mean duration of 15.17ms

## Solution

- ~~Use `Vec` instead of a `SmallVec`~~
- ~~Don't use an temporary variable~~
- Compute the affine matrix as an `Affine3A` instead
- Remove the `temp` vec

| |mean|
|---|---|
|base|15.17ms|
|~~vec~~|~~9.31ms~~|
|~~no temp variable~~|~~11.31ms~~|
|removing the temp vector|8.43ms|
|affine|13.21ms|
|all together|7.23ms|
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cart cart cart approved these changes

@james7132 james7132 james7132 approved these changes

Assignees
No one assigned
Labels
A-Animation Make things move and change over time C-Performance A change motivated by improving speed, memory usage or compile times
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@mockersf @superdump @james7132 @cart