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

Add NES emulation runtime benchmark #1730

Merged
merged 3 commits into from
Oct 21, 2023
Merged

Add NES emulation runtime benchmark #1730

merged 3 commits into from
Oct 21, 2023

Conversation

koute
Copy link
Member

@koute koute commented Sep 30, 2023

This PR adds a new real-world runtime benchmark to the suite: Pinky, my NES emulator.

It uses a simple test ROM I've found here and emulates its execution for a given number of frames (which can be freely adjusted to make the benchmark shorter/longer).

@Kobzol
Copy link
Contributor

Kobzol commented Sep 30, 2023

Great! I have wanted to include some kind of interpreter in the suite, an emulator should do nicely.

I wanted to avoid the git dependency, but then I noticed that we already have a few of them in compile benchmarks, so it's probably not such a big deal.

I wonder if in the benchmark loop, the framebuffer has to be written out? Couldn't we just benchmark the emulation of instructions itself?

@koute
Copy link
Member Author

koute commented Oct 1, 2023

I wonder if in the benchmark loop, the framebuffer has to be written out? Couldn't we just benchmark the emulation of instructions itself?

I added it because technically that is the output of the emulator, but practically speaking it probably doesn't make much difference so I can remove it if you want. (It's just a simple loop which goes over its internal palettized framebuffer, plugs each entry into a lookup table, and writes out the corresponding ABGR color.)

There's also an audio callback that I've omitted, but I'm not sure whether the compiler is smart enough to delete the final parts of the audio processing pipeline if that's empty. Maybe we should black_box both the video and audio outputs just to be sure?

Copy link
Contributor

@Kobzol Kobzol left a comment

Choose a reason for hiding this comment

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

I tried it without the framebuffer and the instruction count stays pretty much the same (~3.4 billion instructions). So I would just remove the framebuffer completely, and return nes from the benchmarking closure instead of the buffer. The returned value is black_boxed automatically by the benchmarking infrastructure.

Other than that one nit, looks good to me! I'll also let @nnethercote take a look.

@Kobzol Kobzol requested a review from nnethercote October 2, 2023 13:41
@Kobzol
Copy link
Contributor

Kobzol commented Oct 2, 2023

Oh, one more thing, it would be nice if you included some short comment either to the top of the main.rs file, or above the benchmark function, to shortly describe what is the benchmark doing.

We should have a list of runtime benchmark descriptions in some README.md (same as for the compilation time benchmarks), but that's not prepared yet.

Copy link
Contributor

@nnethercote nnethercote left a comment

Choose a reason for hiding this comment

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

I think this is a great choice for a benchmark.

collector/runtime-benchmarks/nes/src/main.rs Outdated Show resolved Hide resolved
collector/runtime-benchmarks/nes/src/main.rs Show resolved Hide resolved
@koute
Copy link
Member Author

koute commented Oct 19, 2023

Sorry for the delay!

I simplified the benchmark and added more comments as requested.

Copy link
Contributor

@Kobzol Kobzol left a comment

Choose a reason for hiding this comment

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

Thanks, looks good to me!

I'll let Nick react to his remarks, but I think that this is good to go now.

Copy link
Contributor

@nnethercote nnethercote left a comment

Choose a reason for hiding this comment

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

Lovely, thank you!

@Kobzol Kobzol enabled auto-merge October 21, 2023 06:48
@Kobzol Kobzol merged commit 51403aa into rust-lang:master Oct 21, 2023
10 checks passed
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.

3 participants