[GitHub] [incubator-brpc] jamesge commented on a change in pull request #1200: reduce sampler's mutex scope

[GitBox <gi...@apache.org>]

jamesge commented on a change in pull request #1200:
URL: https://github.com/apache/incubator-brpc/pull/1200#discussion_r525761185



##########
File path: src/bvar/detail/sampler.cpp
##########
@@ -147,31 +147,28 @@ void SamplerCollector::run() {
 
     butil::LinkNode<Sampler> root;
     int consecutive_nosleep = 0;
-    while (!_stop) {
+    while (!_stop.load(butil::memory_order_relaxed)) {
         int64_t abstime = butil::gettimeofday_us();
         Sampler* s = this->reset();
         if (s) {
             s->InsertBeforeAsList(&root);
         }
-        int nremoved = 0;
-        int nsampled = 0;
+
         for (butil::LinkNode<Sampler>* p = root.next(); p != &root;) {
             // We may remove p from the list, save next first.
             butil::LinkNode<Sampler>* saved_next = p->next();
             Sampler* s = p->value();
-            s->_mutex.lock();
-            if (!s->_used) {
-                s->_mutex.unlock();
+            if (s->_used.load(butil::memory_order_acquire)) {
+                BAIDU_SCOPED_LOCK(s->_mutex);

Review comment:
       这个_mutex也不太必要了，可以删掉

##########
File path: src/bvar/detail/sampler.cpp
##########
@@ -202,9 +199,7 @@ void Sampler::schedule() {
 }
 
 void Sampler::destroy() {
-    _mutex.lock();
-    _used = false;
-    _mutex.unlock();
+    _used.store(false, butil::memory_order_release);

Review comment:
       需要在注释里举一个场景说明一下为什么要release语意




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org