[Merged by Bors] - Slight perf improvements and tidy for bevymark by SUPERCILEX · Pull Request #3765 · bevyengine/bevy

SUPERCILEX · 2022-01-25T21:59:03Z

No description provided.

Signed-off-by: Alex Saveau <saveau.alexandre@gmail.com>

alice-i-cecile · 2022-01-25T22:03:46Z

examples/tools/bevymark.rs

        );
-        counter.color = Color::rgb_linear(random(), random(), random());
+
+        let mut random = thread_rng();


Because this is example code, could you please comment to explain why this is faster.

You mean here or as a comment in the code? thread_rng uses an Rc under the hood so theoretically we save some instructions by not cloning it a bunch of times.

Something like

// cache the rng

Might be enough enough here.

alice-i-cecile · 2022-01-25T22:04:41Z

examples/tools/bevymark.rs


 fn movement_system(time: Res<Time>, mut bird_query: Query<(&mut Bird, &mut Transform)>) {
-    for (mut bird, mut transform) in bird_query.iter_mut() {
+    bird_query.for_each_mut(|(mut bird, mut transform)| {


I think that using the for_each pattern here is worth it, but we should explain to beginners that this is faster (and ideally link to an explanation why).

Agreed, I'd actually love to know why this is faster. This PR added it and said it was faster but not why.

Added a note, though I have no idea if it's correct. 😅

I can't comment on the correctness of that note, but I feel like something like

// `.for_each_mut` is faster than `.iter`, but can't be chained like a normal iterator.

would be enough. (based on text here https://docs.rs/bevy/latest/bevy/ecs/system/struct.Query.html#method.for_each_mut)

DJMcNab

Happy with the changes to use for_each, thread_rng, .single and a marker component. I'm conflicted on the change to use write, but I don't want to make the call for that.

The whitespace changes seem bizarre; rustfmt is 'borked' if it thinks the new version is better.

examples/tools/bevymark.rs

Signed-off-by: Alex Saveau <saveau.alexandre@gmail.com>

SUPERCILEX · 2022-02-24T22:55:28Z

@rparrett Would mind taking a look at this PR too? You reviewed #3764

IceSentry

Since you are modifying this example, I would also add a short description of what it is at the top of the file or at least above main(). I would also add the explanation for the .for_each use there.

examples/tools/bevymark.rs

rparrett · 2022-02-25T00:17:15Z

There are likely to be some differing opinions about this. Examples should probably be optimized for clarity, but a "benchmark example" like bevymark seems like a good place to implement / document these sorts of optimizations. So I think this sort of thing is a better fit here than in contributors.

I agree that comments in place where a less-obvious thing is being done for performance reasons would be a good idea.

examples/tools/bevymark.rs

Signed-off-by: Alex Saveau <saveau.alexandre@gmail.com>

examples/tools/bevymark.rs

alice-i-cecile

Looking good. One very small nit + approval from another team member, then I'm happy to merge this.

mockersf · 2022-02-25T16:02:36Z

could you mention the kind of perf improvements you get from those changes?

rparrett · 2022-02-25T16:05:53Z

could you mention the kind of perf improvements you get from those changes?

I don't think it's even really measurable...

alice-i-cecile · 2022-02-25T16:22:39Z

could you mention the kind of perf improvements you get from those changes?

I don't think it's even really measurable...

Testing locally, these changes are maybe a frame or two faster for me at reasonable entity counts. Very hard to measure precisely for this example though.

Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>

rparrett · 2022-02-25T18:46:39Z

Testing locally, these changes are maybe a frame or two faster for me at reasonable entity counts. Very hard to measure precisely for this example though.

On my end, with cargo run --release --example bevymark -- 1000 200, there's a point where fps dips below vsync but bird spawning is still actively happening. In that phase, I see what might be a ~0.5-1 fps improvement. But once spawning stops, it seems too noisy to measure without more effort.

rparrett · 2022-02-25T18:53:06Z

Sidenote: bevymark vsync got re-enabled (accidentally?) with #3812. Will PR separately later if I remember.

Signed-off-by: Alex Saveau <saveau.alexandre@gmail.com>

SUPERCILEX · 2022-02-25T19:17:34Z

@rparrett Fixed

mockersf · 2022-02-25T19:48:00Z

I don't think some of the changes in this PR are really useful as they make the example harder to read if they don't bring the perf improvements from the pr title

SUPERCILEX · 2022-02-25T20:06:12Z

@mockersf Can you point to specifics? The only thing that got more complicated is counter_system and that's not the primary focus of the benchmark.

SUPERCILEX · 2022-02-27T21:25:11Z

@rparrett is there anything you're still hesitant about or is this good for approval?

cart · 2022-02-27T22:43:18Z

I do think the "string allocation optimizations" are on a scale we don't need to worry about here. I agree that the extra complexity isn't worth it. Other than that, I think this is good to go!

Signed-off-by: Alex Saveau <saveau.alexandre@gmail.com>

SUPERCILEX · 2022-02-27T23:29:44Z

Sounds good, done!

mockersf · 2022-02-28T00:40:23Z

also, I wasn't a fan of using for_each_mut, with a comment saying it's faster, when it's not really faster actually

(sorry I took so long replying, I was busy with the jam and missed your reply)

cart · 2022-02-28T00:46:55Z

I was under the impression that for_each_mut() was actually still slightly faster than normal iterators (something like 1.2x), so I'm assuming you mean that it didn't meaningfully impact bevymark perf?

cart · 2022-02-28T00:48:03Z

The gap has definitely closed now that derive(Component) is merged.

SUPERCILEX · 2022-02-28T00:51:33Z

@mockersf Since it's part of the public API, I think it's actually good to showcase different ways of doing things. Also, nitpicking on this level is getting a little excessive IMO: you can't say X is faster in the docs and then complain when people use X. 🤷‍♂️

cart · 2022-02-28T01:01:34Z

@mockersf Since it's part of the public API, I think it's actually good to showcase different ways of doing things. Also, nitpicking on this level is getting a little excessive IMO: you can't say X is faster in the docs and then complain when people use X. man_shrugging

I think its important for examples to encourage "best practices" and "recommend coding styles". I have intentionally pushed for normal query iterators in examples because they are about as fast, more "idiomatic rust", more flexible (because you don't need to refactor to do iterator chaining), and more ergonomic. for_each iterators exist for the (very few) cases where the small perf win matters. If it turns out that bevymark doesn't benefit from it (while iterating hundreds of thousands of entities), thats a pretty good indicator that we really shouldn't be encouraging its use. I appreciate that @mockersf brought this up and this attention to detail is important to me for examples. Unless we can demonstrate real measurable wins for these for_each iterators, they should be swapped out.

Signed-off-by: Alex Saveau <saveau.alexandre@gmail.com>

SUPERCILEX · 2022-02-28T01:16:23Z

That makes sense, thank you for the explanation! I checked with 200K entities and there doesn't seem to be any real difference in FPS, so reverted. Maybe it's worth considering getting rid of those APIs after more careful benchmarking?

cart · 2022-02-28T01:27:17Z

Yeah now that derive(Component) is a thing, its worth having the conversation.

cart · 2022-02-28T01:27:23Z

bors r+

Co-authored-by: Carter Anderson <mcanders1@gmail.com>

bors · 2022-02-28T01:51:07Z

Pull request successfully merged into main.

Build succeeded:

Co-authored-by: Carter Anderson <mcanders1@gmail.com>

Slight perf improvements and tidy for bevymark

a774329

Signed-off-by: Alex Saveau <saveau.alexandre@gmail.com>

github-actions bot added the S-Needs-Triage This issue needs to be labelled label Jan 25, 2022

alice-i-cecile added C-Code-Quality A section of code that is hard to understand or change C-Examples An addition or correction to our examples and removed S-Needs-Triage This issue needs to be labelled labels Jan 25, 2022

alice-i-cecile reviewed Jan 25, 2022

View reviewed changes

DJMcNab reviewed Jan 25, 2022

View reviewed changes

SUPERCILEX added 4 commits January 25, 2022 14:40

Rename random to rng

0d2bebc

Signed-off-by: Alex Saveau <saveau.alexandre@gmail.com>

Smush imports together

6e6158c

Signed-off-by: Alex Saveau <saveau.alexandre@gmail.com>

Only update counter when changed

16622b3

Signed-off-by: Alex Saveau <saveau.alexandre@gmail.com>

Add note about for_each being faster

d2f67e9

Signed-off-by: Alex Saveau <saveau.alexandre@gmail.com>

IceSentry reviewed Feb 24, 2022

View reviewed changes

rparrett reviewed Feb 24, 2022

View reviewed changes

examples/tools/bevymark.rs Outdated Show resolved Hide resolved

rparrett reviewed Feb 25, 2022

View reviewed changes

examples/tools/bevymark.rs Outdated Show resolved Hide resolved

SUPERCILEX added 4 commits February 24, 2022 18:11

Merge remote-tracking branch 'upstream/main' into tmp

bcb1a44

Address review feedback

87dc4d0

Signed-off-by: Alex Saveau <saveau.alexandre@gmail.com>

Oops, add back for_each comment

0db3cdc

Signed-off-by: Alex Saveau <saveau.alexandre@gmail.com>

Use usize instead of u128 so math isn't emulated

212c401

Signed-off-by: Alex Saveau <saveau.alexandre@gmail.com>

alice-i-cecile reviewed Feb 25, 2022

View reviewed changes

examples/tools/bevymark.rs Outdated Show resolved Hide resolved

alice-i-cecile reviewed Feb 25, 2022

View reviewed changes

examples/tools/bevymark.rs Outdated Show resolved Hide resolved

alice-i-cecile requested changes Feb 25, 2022

View reviewed changes

SUPERCILEX and others added 2 commits February 25, 2022 09:54

Update examples/tools/bevymark.rs

a2374b4

Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>

Update examples/tools/bevymark.rs

f5c015e

Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>

Disable vsync

3146c53

Signed-off-by: Alex Saveau <saveau.alexandre@gmail.com>

alice-i-cecile approved these changes Feb 25, 2022

View reviewed changes

Address feedback

0e3a0d3

Signed-off-by: Alex Saveau <saveau.alexandre@gmail.com>

remove old comment

37a8120

Go back to iter

ddd219a

Signed-off-by: Alex Saveau <saveau.alexandre@gmail.com>

bors bot pushed a commit that referenced this pull request Feb 28, 2022

Slight perf improvements and tidy for bevymark (#3765)

461cf53

Co-authored-by: Carter Anderson <mcanders1@gmail.com>

bors bot changed the title ~~Slight perf improvements and tidy for bevymark~~ [Merged by Bors] - Slight perf improvements and tidy for bevymark Feb 28, 2022

bors bot closed this Feb 28, 2022

alice-i-cecile mentioned this pull request Feb 28, 2022

Remove for_each and for_each_mut APIs #4059

Closed

kurtkuehnert pushed a commit to kurtkuehnert/bevy that referenced this pull request Mar 6, 2022

Slight perf improvements and tidy for bevymark (bevyengine#3765)

77220cb

Co-authored-by: Carter Anderson <mcanders1@gmail.com>

SUPERCILEX deleted the tmp branch May 17, 2022 04:21

Uh oh!

Conversation

SUPERCILEX commented Jan 25, 2022 • edited by bors bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alice-i-cecile Jan 25, 2022

Choose a reason for hiding this comment

Uh oh!

SUPERCILEX Jan 25, 2022

Choose a reason for hiding this comment

Uh oh!

rparrett Feb 25, 2022

Choose a reason for hiding this comment

Uh oh!

alice-i-cecile Jan 25, 2022

Choose a reason for hiding this comment

Uh oh!

SUPERCILEX Jan 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SUPERCILEX Feb 24, 2022

Choose a reason for hiding this comment

Uh oh!

rparrett Feb 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DJMcNab left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

SUPERCILEX commented Feb 24, 2022

Uh oh!

IceSentry left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rparrett commented Feb 25, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

alice-i-cecile left a comment

Choose a reason for hiding this comment

Uh oh!

mockersf commented Feb 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rparrett commented Feb 25, 2022

Uh oh!

alice-i-cecile commented Feb 25, 2022

Uh oh!

rparrett commented Feb 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rparrett commented Feb 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SUPERCILEX commented Feb 25, 2022

Uh oh!

mockersf commented Feb 25, 2022

Uh oh!

SUPERCILEX commented Feb 25, 2022

Uh oh!

SUPERCILEX commented Feb 27, 2022

Uh oh!

cart commented Feb 27, 2022

Uh oh!

SUPERCILEX commented Feb 27, 2022

Uh oh!

mockersf commented Feb 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cart commented Feb 28, 2022

SUPERCILEX commented Jan 25, 2022 •

edited by bors bot

Loading

SUPERCILEX Jan 25, 2022 •

edited

Loading

rparrett Feb 25, 2022 •

edited

Loading

DJMcNab left a comment •

edited

Loading

mockersf commented Feb 25, 2022 •

edited

Loading

rparrett commented Feb 25, 2022 •

edited

Loading

rparrett commented Feb 25, 2022 •

edited

Loading

mockersf commented Feb 28, 2022 •

edited

Loading

SUPERCILEX commented Feb 28, 2022 •

edited

Loading