-
Notifications
You must be signed in to change notification settings - Fork 187
starboard: Refactor PlayerWorker to use JobThread #9027
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
Conversation
🤖 Gemini Suggested Commit Message💡 Pro Tips for a Better Commit Message:
|
This change replaces the manual Thread and JobQueue management in PlayerWorker with JobThread, simplifying the destruction sequence and task scheduling. Bug: 468356861
5471e66 to
852c95f
Compare
|
/gemini review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request refactors PlayerWorker to use JobThread, which significantly simplifies thread management and the class's destruction sequence. The changes are well-executed, removing manual thread and job queue handling in favor of the JobThread abstraction. This also allowed for making many member variables const, improving the class's immutability and making it easier to reason about. The replacement of SB_DCHECK with SB_CHECK for thread-safety assertions is a good improvement for correctness. I have a few suggestions to further improve maintainability by using lambdas instead of std::bind, in line with modern C++ best practices and the Chromium style guide.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request is a great improvement, refactoring PlayerWorker to use JobThread for thread and task management. This significantly simplifies the implementation, especially in the constructor and destructor, by removing manual thread synchronization logic. The increased use of const for member variables enhances robustness and code clarity. The switch from std::bind to lambdas for scheduling tasks is a welcome modernization. I have one suggestion to replace the remaining std::bind calls with lambdas for consistency.
rakuco
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm with a few suggestions. Thanks for picking this up!
This change replaces the manual Thread and JobQueue management in
PlayerWorker with JobThread. This simplifies the destruction sequence
and task scheduling within the player worker, improving code clarity
and robustness.
Bug: 468356861