Avoid calling destroy event inside host func - #278
Conversation
Summary of ChangesHello @MC952-arch, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request refactors the event and semaphore lifecycle management within the Highlights
Ignored Files
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. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the lifecycle management of flagcxHostSemaphore to avoid destroying events inside the host function. The ownership of the semaphore is transferred to the proxy progress thread, which now correctly waits for the host function to complete before cleaning up.
My review has identified a critical issue with the new implementation. The flagcxHostSemaphore struct contains a std::vector, making it a non-POD C++ object. However, it's being allocated with flagcxCalloc and deallocated with free. This is incorrect as it bypasses constructor and destructor calls, leading to memory leaks of both the events and the vector's internal buffer. I've provided suggestions to fix this by using new and delete for proper C++ object lifecycle management.
Additionally, I've pointed out a minor issue regarding a potentially unsafe cast from size_t to int.
|
@gemini-code-assist please review this pull request in details |
There was a problem hiding this comment.
Code Review
This pull request refactors event management in flagcxHostSemaphore to improve stability by avoiding event destruction within host functions. The core change moves the event cleanup logic into the semaphore's destructor, which is now invoked from proxy.cc after the host function signals completion. This is a good architectural improvement. My review includes a critical fix for a memory management mismatch (calloc vs. delete) that could lead to undefined behavior, along with suggestions to improve code clarity and remove redundancy.
|
@gemini-code-assist please review this pull request in details |
There was a problem hiding this comment.
Code Review
This pull request aims to improve the stability of the flagcxHostSemaphore by managing event destruction in a context-safe manner. The changes include replacing flagcxCalloc with new for semaphore allocation, introducing signalStart and signalEnd methods, and handling event destruction in the semaphore's destructor. Additionally, an eventId is added to flagcxProxyOp to track semaphore events. The polling mechanism in flagcxProxySend and flagcxProxyRecv is updated to use pollStart. Finally, the semaphore deletion logic is adjusted to ensure only the last operation within a group releases the semaphore. I have identified a critical issue related to potential double freeing of the semaphore, which needs to be addressed.
| if (op->args.semaphore->pollEnd()) { | ||
| delete op->args.semaphore; | ||
| flagcxIntruQueueDelete(queue, op); | ||
| free(op); | ||
| } |
There was a problem hiding this comment.
The condition op->args.semaphore->pollEnd() is checked before deleting the semaphore. However, op->args.semaphore might already be deleted in another thread if progressOps is executed concurrently for different operations belonging to the same semaphore. This could lead to a double-free scenario and crash the application.
To fix this, consider using a synchronization mechanism (e.g., a mutex or atomic flag) to ensure that only one thread can delete the semaphore at a time. Also, add a check to ensure that op->args.semaphore is not null before attempting to access it.
if (op->args.semaphore) { // Check if semaphore is not null
if (op->args.semaphore->pollEnd()) {
delete op->args.semaphore;
op->args.semaphore = nullptr; // Set to null after deletion
flagcxIntruQueueDelete(queue, op);
free(op);
}
}
|
@gemini-code-assist please review this pull request in details |
There was a problem hiding this comment.
Code Review
This pull request refactors event management to improve stability by moving event destruction into the flagcxHostSemaphore destructor, managed by a std::shared_ptr. The changes are logical and move towards safer resource management. However, I've found a critical issue related to memory management of flagcxProxyOp which now contains a non-POD std::shared_ptr member but is still allocated with calloc and free. This must be fixed to prevent memory leaks and ensure destructors are called. I've also included a suggestion to further improve the flagcxHostSemaphore implementation for better C++ idiomaticity and robustness.
|
@gemini-code-assist please review this pull request in details |
There was a problem hiding this comment.
Code Review
This pull request refactors event management in flagcxHostSemaphore to prevent device context errors from destroying events inside host functions. The change to using std::shared_ptr for flagcxHostSemaphore is a great improvement, leveraging RAII for safer resource management. The logic is sound and correctly implemented across all modified files, enhancing both safety and code clarity. I have one suggestion to further improve the robustness of the flagcxHostSemaphore struct.
3 participants
This change refactors event management in flagcxHostSemaphore to prevent device context errors that may occur when destroying events inside host functions. Since host function behavior varies across different AI chips, event destruction is now handled in a context-safe manner to ensure consistent cross-platform stability.