[GitHub] [celix] PengZheng commented on pull request #376: Eliminate unnecessary blocking and mutext from useServiceWithOptions.

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

PengZheng commented on pull request #376:
URL: https://github.com/apache/celix/pull/376#issuecomment-965961677


   Creating tracker on the event loop elegantly solves the problem of getting all existent services without any race condition.
   I like this new approach very much! However, the "use" on the event loop is another story, the current implementation is expensive when high resolution timer is enabled on Linux:
   
   ```C
       do {
           eventId = celix_framework_fireGenericEvent(ctx->framework, -1, celix_bundle_getId(ctx->bundle), "use service tracker for celix_bundleContext_useServiceWithOptions", &data, celix_bundleContext_useServiceWithOptions_2_UseServiceTracker, NULL, NULL);
           celix_framework_waitForGenericEvent(ctx->framework, 
           if (!useServiceIsDone) {
               usleep(10);      //This might be too short when HR timer is enabled
           }
       } while (!useServiceIsDone);
   ```
   I'm thinking about this and planning another PR, which will be posted when I've considered all corner cases. Anyway, this should be the topic of another thread.
   
   I agree that the overhead of locking/unlocking the mutex when accessing `called` and `count` is insignificant. I'm OK keeping them if you insist. I realized with the mutex removed, these celix_bundleContext_useServiceWithOptions_x utility functions can be reused directly, i.e. introducing as little modification as possible. The only thing I have to do is to convince you that it will cause no problem ;)
   
   Let me borrow Listing 5.2. (Reading and writing variables from different threads) from C++ Concurrency in Action:
   
   ```C++
   #include <vector>
   #include <atomic>
   #include <iostream>
   std::vector<int> data;
   std::atomic<bool> data_ready(false);
   void reader_thread()
   {
       while(!data_ready.load())                   //1
       {
           std::this_thread::sleep(std::chrono::milliseconds(1));
       }
       std::cout<<"The answer="<<data[0]<<"\n";   //2
   }
   void writer_thread()
   {
       data.push_back(42);                         //3
       data_ready=true;                            //4
   }
   ```
   
   > The required enforced ordering comes from the operations on the std:: atomic<bool> variable, data_ready;, they provide the necessary ordering by virtue of the memory model relations happens-before and synchronizes-with. The write of the data 3 happens before the write to the data_ready flag 4, and the read of the flag 1 happens before the read of the data 2. When the value read from data_ready 1 is true, the write synchronizes with that read, creating a happens-before relationship. Because happens-before is transitive, the write to the data 3 happens before the write to the flag 4, which happens before the read of the true value from the flag 1, which happens before the read of the data 2, and you have an enforced ordering: the write of the data happens before the read of the data and everything is OK. Figure 5.2 shows the important happens-before relationships in the two threads.
   
   Using the book's term, we order non-atomic operations on `data` through the use of atomic operations on `data_ready`. Then I made little modification to the above example to mimic our code's behavior:
   
   ```C++
   #include <vector>
   #include <atomic>
   #include <iostream>
   std::atomic<bool> data_ready(false);
   bool called; //for use service
   size_t count; //for use services
   
   void reader_thread()
   {
       while(!data_ready.load())                   //1
       {
           std::this_thread::sleep(std::chrono::milliseconds(1));
       }
       std::cout<<"The answer="<<called<< count <<"\n";   //2
   }
   void writer_thread()
   {
       called = true;
       count = 3;
       data_ready=true;                            //4
   }
   ```
   If we agree the quoted example is correct, then it's straightforward to see the modified one is also correct. To see why my patch is correct, we need to make sure that POSIX mutex provides similar higher level semantics to atomic-operations, which is indeed the case. Again, I quote the book:
   
   >std::mutex, std::timed_mutex, std::recursive_mutex, std::recursive_timed_mutex: All calls to lock and unlock, and successful calls to try_lock, try_lock_for, or try_lock_until, on a given mutex object form a single total order: the lock order of the mutex. A call to unlock on a given mutex object synchronizes with a subsequent call to lock, or a subsequent successful call to try_lock, try_lock_for, or try_lock_until, on that object in the lock order of the mutex. Failed calls to try_lock, try_lock_for, or try_lock_until do not participate in any synchronization relationships.
   
   > std::condition_variable and std::condition_variable_any: Condition variables do not provide any synchronization relationships. They are optimizations over busy-wait loops, and all the synchronization is provided by the operations on the associated mutex.
   
   Therefore, I claim the mutex is unnecessary because `celix_framework_waitForGenericEvent` already provides all synchronization we need (through the mutex semantics). Pthread library really make multi-threading much more intuitive, otherwise the concurrency world is rocket science.
   
   Please forgive this non-native English speaker if I failed to explain my thoughts clearly.
   


-- 
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: dev-unsubscribe@celix.apache.org

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