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

Put asset_events behind a run condition #11800

Merged
merged 2 commits into from
Feb 12, 2024

Conversation

james7132
Copy link
Member

@james7132 james7132 commented Feb 9, 2024

Objective

Scheduling low cost systems has significant overhead due to task pool contention and the extra machinery to schedule and run them. Following the example of #7728, asset_events is good example of this kind of system, where there is no work to be done when there are no queued asset events.

Solution

Put a run condition on it that checks if there are any queued events.

Performance

Tested against many_foxes, we can see a slight improvement in the total time spent in UpdateAssets. Also noted much less volatility due to not being at the whim of the OS thread scheduler.
image

@james7132 james7132 added A-Assets Load files from disk to use for things like images, models, and sounds C-Performance A change motivated by improving speed, memory usage or compile times labels Feb 9, 2024
@james7132 james7132 changed the title Put asset_events behind a run conditon Put asset_events behind a run condition Feb 10, 2024
@hymm hymm self-requested a review February 10, 2024 02:16
Copy link
Contributor

@BorisBoutillier BorisBoutillier left a comment

Choose a reason for hiding this comment

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

Do we need to do a thorough pass on all bevy systems to check their internal complexity and if there is a pertinent run condition to add, or is this still a compromise and always need to be checked against a benchmark ?

@james7132
Copy link
Member Author

james7132 commented Feb 10, 2024

Do we need to do a thorough pass on all bevy systems to check their internal complexity and if there is a pertinent run condition to add, or is this still a compromise and always need to be checked against a benchmark ?

The main goal here is performance so I'd always measure the results to ensure we don't have any regressions. Run conditions are always done on a single thread and block scheduling new system tasks, so there is a tradeoff to be made here when we're talking a large number of systems.

@hymm
Copy link
Contributor

hymm commented Feb 10, 2024

Is the Tracy with the system level spans disabled? That could significantly change the results.

@james7132
Copy link
Member Author

No. I didn't disable system or run condition spans for either run. I'll go ahead and disable both and see how it goes.

Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>
@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 Feb 10, 2024
@alice-i-cecile
Copy link
Member

I'm curious about those perf results, but won't block on them :)

@james7132
Copy link
Member Author

james7132 commented Feb 10, 2024

Is the Tracy with the system level spans disabled? That could significantly change the results.

This is the comparison for the AssetEvents schedule with the system spans commented out. Disabling the system spans shows an even bigger improvement. I disabled it both on this PR and main.

image

A significant portion of that last ~40us is likely attributable to executor/thread wake-up overhead, which should be addressed by #11801.

image

Copy link
Contributor

@hymm hymm left a comment

Choose a reason for hiding this comment

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

Thanks for redoing the benches.

@alice-i-cecile alice-i-cecile added this pull request to the merge queue Feb 12, 2024
Merged via the queue into bevyengine:main with commit eee71bf Feb 12, 2024
24 checks passed
@james7132 james7132 deleted the asset-event-run-condition branch March 10, 2024 07:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Assets Load files from disk to use for things like images, models, and sounds C-Performance A change motivated by improving speed, memory usage or compile times 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.

4 participants