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

Improve Simulator CPU Usage up to 50%. #45

Merged
merged 6 commits into from
Mar 6, 2023
Merged

Improve Simulator CPU Usage up to 50%. #45

merged 6 commits into from
Mar 6, 2023

Conversation

grantshandy
Copy link
Contributor

@grantshandy grantshandy commented Jan 20, 2023

Description

This PR includes some minor changes to OutputSettings, Window, and SdlWindow which greatly improve the performance of the embedded graphics simulator.

Changes

Create TextureCreator and Texture at the initialization of SdlWindow, not on each frame.

@rfuest mentioned that the texture creator and texture had to be created on each frame because of a lifetime issue. I encountered this "self-referencing lifetime memory issue" too, but was able to overcome it by using the proc macro ouroboros on a new private struct SdlWindowTexture.

Limit FPS to 60Hz default.

One of the main factors that led to simulator programs usually taking ~100% of a CPU core was letting the program "run wild in the hot loop" by default. Now the program puts the thread to sleep if the draw took less than the desired fps. I set the default FPS limit as 60Hz, but this can be changed in OutputSettings. I imagine most programs written with the embedded-graphics framework in mind aren't FPS-dependent, but if they are I'm sure they can set the max fps to much higher in order to eliminate this effect.

Performance Improvements

I've found that with the input-handling example, the changes in this PR improve my CPU usage by ~50%!

Here are the results of ps -p <pid> -o %cpu,%mem,cmd on the master branch:

%CPU %MEM CMD
99.5  0.8 target/release/examples/input-handling

And on my fork:

%CPU %MEM CMD
49.7  0.8 target/release/examples/input-handling

Copy link
Member

@jamwaffles jamwaffles left a comment

Choose a reason for hiding this comment

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

@rfuest any comments before I merge this?

@rfuest
Copy link
Member

rfuest commented Mar 6, 2023

No, LGTM. The only thing that is missing is a changelog entry for the new setting and the changed behavior of update.

Copy link
Member

@jamwaffles jamwaffles left a comment

Choose a reason for hiding this comment

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

Ah of course. @grantshandy under the Unreleased section in CHANGELOG.md, could you please add the following (or similar if you want to word it differently):

### Added

- [#45](https://github.com/embedded-graphics/simulator/pull/45) Added `OutputSettingsBuilder::max_fps` to set the maximum FPS of the simulator.
  
### Changed

- [#45](https://github.com/embedded-graphics/simulator/pull/45) Limit simulator to 60FPS by default.

sdl_window.update(&framebuffer);
}

thread::sleep(start + self.desired_loop_duration - Instant::now());
Copy link
Member

@bugadani bugadani Mar 6, 2023

Choose a reason for hiding this comment

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

This sleep assumes my processing takes 0 time. Can you store the time after the thread wakes up, then use that as a reference instead of grabbing it at the start of update, please?

Also, in that case, my processing may take a long time and it will be possible for the sleep time to be "negative", in which case what happens is currently rust version dependent, as stated here. Could you please turn this into a checked subtraction?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for noticing this! I'll apply these suggestions and add the section in CHANGELOG.md that @jamwaffles suggested this afternoon. 👍🏽

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thread::sleep(
    (self.frame_start + self.desired_loop_duration)
        .saturating_duration_since(Instant::now()),
);

self.frame_start = Instant::now();

If your frame takes a long time and becomes "negative", saturating_duration_since will return zero, which I think will fix the issue.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for decoding my awkward comment perfectly :)

@grantshandy
Copy link
Contributor Author

I made a mistake naming this commit (CONTRIBUTING.md, not CHANGELOG.md), but it should address the issues @rfuest and @bugadani brought up.

Copy link
Member

@jamwaffles jamwaffles left a comment

Choose a reason for hiding this comment

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

Looks good. Thanks very much!

@jamwaffles jamwaffles merged commit 11ec451 into embedded-graphics:master Mar 6, 2023
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.

Bad performance when animating
4 participants