Skip to content

Add thread-safety documentation and -O3 optimization#3

Merged
olaugh merged 3 commits intomainfrom
thread-safety-docs
Jan 20, 2026
Merged

Add thread-safety documentation and -O3 optimization#3
olaugh merged 3 commits intomainfrom
thread-safety-docs

Conversation

@olaugh
Copy link
Owner

@olaugh olaugh commented Jan 20, 2026

  • Document that Genesis Plus GX is not thread-safe due to global state
  • Add examples of safe (fork-based) and unsafe (thread-based) parallelism
  • Enable -O3 optimization for faster test execution

- Document that Genesis Plus GX is not thread-safe due to global state
- Add examples of safe (fork-based) and unsafe (thread-based) parallelism
- Enable -O3 optimization for faster test execution

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR adds comprehensive documentation about thread-safety concerns in Genesis Plus GX and enables -O3 compiler optimization for faster test execution. The documentation clearly warns users that the emulator uses global state and cannot be safely used with thread-based parallelism.

Changes:

  • Added extensive thread-safety warnings and documentation with examples of safe (fork-based) and unsafe (thread-based) parallelism patterns
  • Enhanced class-level documentation for the Emulator class to emphasize non-thread-safe behavior
  • Enabled -O3 optimization flag for the Genesis Plus GX core library to improve test execution performance

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
include/gxtest.h Added detailed thread-safety documentation with fork-based and thread-based examples, and enhanced Emulator class documentation
BUILD.bazel Added -O3 compiler optimization flag to improve emulator performance during test execution

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

"vendor/genplusgx/cd_hw/*.h",
]) + ["src/osd.h"],
copts = [
"-O3", # Always optimize emulator for fast test execution
Copy link

Copilot AI Jan 20, 2026

Choose a reason for hiding this comment

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

The -O3 optimization flag is set unconditionally with the comment "Always optimize emulator for fast test execution". While this is beneficial for test execution speed, it may hinder debugging efforts when issues arise in the emulator core. Consider whether this should respect the build configuration mode (e.g., using select() to apply -O3 only in optimized builds, or opt builds) to preserve debuggability in debug configurations.

Copilot uses AI. Check for mistakes.
include/gxtest.h Outdated
Comment on lines +20 to +25
* if (pid == 0) {
* // Child process - has its own copy of global state
* GX::Emulator emu;
* emu.LoadRom("game.bin");
* emu.RunFrames(1000);
* // Write results to pipe, exit
Copy link

Copilot AI Jan 20, 2026

Choose a reason for hiding this comment

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

The fork-based example is incomplete and lacks proper error handling. The child process should explicitly call exit() or _exit() after completing its work to prevent it from continuing into the parent's code path. Additionally, the example doesn't show error handling for fork() failures (when it returns -1). Consider adding "exit(0);" after the "Write results to pipe" comment and showing error handling for the fork() call to make this a more complete and accurate example.

Suggested change
* if (pid == 0) {
* // Child process - has its own copy of global state
* GX::Emulator emu;
* emu.LoadRom("game.bin");
* emu.RunFrames(1000);
* // Write results to pipe, exit
* if (pid < 0) {
* // Handle fork() error
* perror("fork");
* exit(1);
* } else if (pid == 0) {
* // Child process - has its own copy of global state
* GX::Emulator emu;
* emu.LoadRom("game.bin");
* emu.RunFrames(1000);
* // Write results to pipe, then exit child
* exit(0);

Copilot uses AI. Check for mistakes.
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +29 to +30
* }
* // Parent collects results from children
Copy link

Copilot AI Jan 20, 2026

Choose a reason for hiding this comment

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

The fork-based example is incomplete and may mislead developers. It's missing several critical elements:

  1. The child process should call exit() or _exit() to prevent it from continuing execution after the emulator work
  2. The parent process should wait for the child using waitpid() or similar to prevent zombie processes
  3. Error handling for fork() failure (when it returns -1) is not shown

Consider adding a complete example that shows proper process management, or add a comment noting that the example is simplified and production code should include proper error handling and process cleanup.

Suggested change
* }
* // Parent collects results from children
* } else {
* int status = 0;
* if (waitpid(pid, &status, 0) < 0) {
* perror("waitpid"); // Handle waitpid() error
* }
* // Parent collects results from children
* }

Copilot uses AI. Check for mistakes.
- Add profiler.h/cpp with per-function cycle counting
- Enable HOOK_CPU to hook into Genesis Plus GX instruction execution
- Support Simple mode (fast) and CallStack mode (inclusive cycles)
- Load symbols from ELF files via nm
- Fix cpuhook.h extern declaration for cpu_hook

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
@olaugh olaugh merged commit 778b2eb into main Jan 20, 2026
4 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.

2 participants