-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Fix race condition caused by mstats replacing it with task_info #15501 #15595
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
base: main
Are you sure you want to change the base?
Conversation
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. |
FirebasePerformance/Sources/Gauges/Memory/FPRMemoryGaugeCollector.m
Outdated
Show resolved
Hide resolved
|
/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 addresses a race condition by replacing mstats() with the more modern task_info() for memory metric collection, which is a great improvement for stability. My review focuses on improving the clarity and maintainability of the new implementation. I've suggested removing a now-redundant variable and a block of dead code. I also noted that the collected memory metric has changed semantically from heap usage to physical memory footprint, and recommended updating internal data structures and comments to reflect this to avoid future confusion. Overall, these changes are positive, and addressing the feedback will further enhance the code quality.
FirebasePerformance/Sources/Gauges/Memory/FPRMemoryGaugeCollector.m
Outdated
Show resolved
Hide resolved
|
/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 addresses a race condition in memory metric collection by replacing the usage of mstats with the more modern and thread-safe task_info API. This is a solid improvement for stability. The implementation correctly uses task_info to get the physical memory footprint. My review includes a suggestion to improve error logging and raises a point about the naming of the collected metric, which has semantically changed from heap usage to physical memory footprint, potentially causing confusion.
…tor.m Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
|
/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 effectively addresses a race condition by replacing the mstats() call with the more modern and thread-safe task_info() for memory usage measurement. This is a great improvement for the stability of the SDK. The change from measuring heap usage to physical memory footprint is clearly communicated through updated comments, and the removal of the now-unavailable heapAvailable metric is handled correctly. The implementation is solid, and I have just one minor suggestion to further improve type safety.
FirebasePerformance/Sources/Gauges/Memory/FPRMemoryGaugeCollector.m
Outdated
Show resolved
Hide resolved
…tor.m Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
This fix consists of using task_info rather than mstats which is a more modern way to measure memory usage, task_info should do a memory query that does not touch the allocator's internal state
This should fix issue #15501
Testing
API Changes