Skip to content

Fair Change Detection Benchmarking#11173

Merged
alice-i-cecile merged 11 commits intobevyengine:mainfrom
re0312:more-change-detection-benchmark
Jun 26, 2024
Merged

Fair Change Detection Benchmarking#11173
alice-i-cecile merged 11 commits intobevyengine:mainfrom
re0312:more-change-detection-benchmark

Conversation

@re0312
Copy link
Contributor

@re0312 re0312 commented Jan 1, 2024

Objective

  • [Merged by Bors] - Change Detection Benchmarks #4972 introduce a benchmark to measure chang detection performance
  • However,it uses iter_batch cause a lot of overhead in clone data to each routine closure(it feels like a bug initer_batch ) and constructs new query in every iter.This overhead masks the real change detection throughput we want to measure. Instead of evaluating raw change detection, the benchmark ends up dominated by data cloning and allocation costs.

Solution

  • Use iter_batch_ref to reduce the benchmark overload
  • Use cached query to better reflect real-world usage scenarios.
  • Add more benmark

Changelog

@alice-i-cecile alice-i-cecile added A-ECS Entities, components, systems, and events C-Performance A change motivated by improving speed, memory usage or compile times labels Jan 2, 2024
Copy link
Member

@james7132 james7132 left a comment

Choose a reason for hiding this comment

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

This looks well motivated and the reasoning works out, but I'd like to see how much of an impact this has on the final benchmark numbers. This isn't to say the PR will be blocked on them changing in a particular direction, but it's best we show what impact this has.

@re0312
Copy link
Contributor Author

re0312 commented Jan 2, 2024

This looks well motivated and the reasoning works out, but I'd like to see how much of an impact this has on the final benchmark numbers. This isn't to say the PR will be blocked on them changing in a particular direction, but it's best we show what impact this has.

I do a benchmark in main and this pr,results are as follows

main (entities from 50000 to 60000 ,the detection time increase 2x~3x,this appears to be an unreasonable outcome.)
image

pr(added the 60000 entities benchmark to compare)
image
time increase linear with entity count

@alice-i-cecile alice-i-cecile added C-Benchmarks Stress tests and benchmarks used to measure how fast things are and removed C-Performance A change motivated by improving speed, memory usage or compile times labels Jan 4, 2024
@alice-i-cecile alice-i-cecile added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Jun 26, 2024
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Jun 26, 2024
Merged via the queue into bevyengine:main with commit f0bdce7 Jun 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-ECS Entities, components, systems, and events C-Benchmarks Stress tests and benchmarks used to measure how fast things are S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants