Copilot commented on code in PR #3637:
URL: https://github.com/apache/brpc/pull/3637#discussion_r4336880775
##########
src/bthread/sys_futex.cpp:
##########
@@ -22,29 +22,43 @@
#include "bthread/sys_futex.h"
#include "butil/scoped_lock.h"
#include "butil/atomicops.h"
+#include "butil/containers/linked_list.h"
#include <pthread.h>
#include <unordered_map>
#if defined(OS_MACOSX)
namespace bthread {
-class SimuFutex {
-public:
- SimuFutex() : counts(0)
- , ref(0) {
- pthread_mutex_init(&lock, nullptr);
+struct SimuFutexWaiter : public butil::LinkNode<SimuFutexWaiter> {
+ SimuFutexWaiter() : signaled(false) {
pthread_cond_init(&cond, nullptr);
}
Review Comment:
This change introduces `pthread_cond_init`/`pthread_cond_destroy` per wait
operation (previously there was a single condvar per `SimuFutex`). That can be
a measurable overhead on hot paths. If this simulated futex is
performance-sensitive, consider alternatives such as reusing waiter objects via
a small pool/free-list (so condvars are reused), or retaining a shared condvar
with a queue + per-waiter token/sequence to preserve correct wake accounting.
##########
src/bthread/sys_futex.cpp:
##########
@@ -22,29 +22,43 @@
#include "bthread/sys_futex.h"
#include "butil/scoped_lock.h"
#include "butil/atomicops.h"
+#include "butil/containers/linked_list.h"
#include <pthread.h>
#include <unordered_map>
#if defined(OS_MACOSX)
namespace bthread {
-class SimuFutex {
-public:
- SimuFutex() : counts(0)
- , ref(0) {
- pthread_mutex_init(&lock, nullptr);
+struct SimuFutexWaiter : public butil::LinkNode<SimuFutexWaiter> {
+ SimuFutexWaiter() : signaled(false) {
pthread_cond_init(&cond, nullptr);
}
+ ~SimuFutexWaiter() {
+ // The futex mutex is still held when the stack waiter is destroyed.
+ // Notified waiters were detached by futex_wake_private. Remove only
+ // waiters that leave on timeout or error.
+ if (!signaled) {
+ RemoveFromList();
+ }
+ pthread_cond_destroy(&cond);
+ }
Review Comment:
The destructor has a non-obvious side effect (mutating the intrusive list)
and depends on a specific locking/lifetime invariant (“mutex is still held when
destroyed”). That’s fragile if this function is refactored later. Prefer to
explicitly `RemoveFromList()` in `futex_wait_private()` on non-signaled exits
(timeout/error) while the lock is definitely held, and keep the destructor
limited to resource cleanup (`pthread_cond_destroy`).
##########
test/bthread_futex_unittest.cpp:
##########
@@ -19,6 +19,7 @@
#include <unistd.h>
#include <stdio.h>
#include <signal.h>
+#include <pthread.h>
Review Comment:
`INT_MAX` is used in the new tests but this file doesn’t include
`<climits>`/`<limits.h>` in the shown include list. Relying on transitive
includes is non-portable and can break compilation on some toolchains—add
`#include <climits>` (or `<limits.h>`) explicitly.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
