You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@celix.apache.org by GitBox <gi...@apache.org> on 2021/11/06 05:09:06 UTC
[GitHub] [celix] PengZheng opened a new pull request #376: Eliminate unnecessary blocking and mutext from useServiceWithOptions.
PengZheng opened a new pull request #376:
URL: https://github.com/apache/celix/pull/376
Currently calling 'celix_bundleContext_useServiceWithOptions' with non-zero timeout
for a non-existent service will prevent the the Celix event loop from registering any new service.
Thus 'celix_bundleContext_useServiceWithOptions' blocks in vain. This PR fixed this and removed
the unnecessary mutex from 'celix_bundle_context_use_service_data_t'. By removing the mutex,
there is no need to destroy the mutex anymore.
Why the mentioned mutex is unnecessary?
In the language of C/C++11 memory model, `celixThreadMutex_unlock(&fw->dispatcher.mutex)` in `fw_removeTopEventFromQueue` **synchronizes with** `celixThreadMutex_lock(&fw->dispatcher.mutex)`
in `celix_framework_waitForGenericEvent` and the implicit lock when `celixThreadCondition_timedwaitRelative` returns.
Note also that `fw_handleEventRequest` **happens before** `fw_removeTopEventFromQueue` in the event loop
thread and that `celix_framework_waitForGenericEvent` **happens before** what follows in the calling thread.
By the above three, we can safely conclude that `fw_handleEventRequest` happens before what follows `celix_framework_waitForGenericEvent` in the calling thread. See [C++ Concurrency in Action, Second Edition, Chapter 5](https://learning.oreilly.com/library/view/c-concurrency-in/9781617294693/kindle_split_015.html#ch05lev1sec3).
Please correct me if I'm wrong.
--
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
[GitHub] [celix] pnoltes commented on pull request #376: Eliminate unnecessary blocking and mutext from useServiceWithOptions.
Posted by GitBox <gi...@apache.org>.
pnoltes commented on pull request #376:
URL: https://github.com/apache/celix/pull/376#issuecomment-974823133
I will leave this PR open a bit longer, so that other Celix committers can also review this.
--
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
[GitHub] [celix] PengZheng edited a comment on pull request #376: Eliminate unnecessary blocking and mutext from useServiceWithOptions.
Posted by GitBox <gi...@apache.org>.
PengZheng edited a comment 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.
PS: C version explanation is given in Chapter 19 of [Modern C](https://gustedt.gitlabpages.inria.fr/modern-c/), which is freely available. C++11 and C11 memory model is essentially the same thing, thus my argument above also applies to C.
--
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
[GitHub] [celix] pnoltes merged pull request #376: Eliminate unnecessary blocking and mutext from useServiceWithOptions.
Posted by GitBox <gi...@apache.org>.
pnoltes merged pull request #376:
URL: https://github.com/apache/celix/pull/376
--
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
[GitHub] [celix] pnoltes commented on pull request #376: Eliminate unnecessary blocking and mutext from useServiceWithOptions.
Posted by GitBox <gi...@apache.org>.
pnoltes commented on pull request #376:
URL: https://github.com/apache/celix/pull/376#issuecomment-969227237
> The only thing I have to do is to convince you that it will cause no problem ;)
lol
Very well written explanation. What is new to me is the `synchronizes-with` part, but seeing this I think reading up to do.
I now agree that the patch is correct. The only worry I have left, is whether relying on the use of mutexes/atomics in "downstream" functions make the code more complex to maintain.
But for this PR, I will approve it :)
--
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
[GitHub] [celix] PengZheng commented on pull request #376: Eliminate unnecessary blocking and mutext from useServiceWithOptions.
Posted by 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
[GitHub] [celix] codecov-commenter edited a comment on pull request #376: Eliminate unnecessary blocking and mutext from useServiceWithOptions.
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #376:
URL: https://github.com/apache/celix/pull/376#issuecomment-962397253
# [Codecov](https://codecov.io/gh/apache/celix/pull/376?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#376](https://codecov.io/gh/apache/celix/pull/376?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (f6af626) into [master](https://codecov.io/gh/apache/celix/commit/bb9179a68fdadda8a4ad3cf79cb7ded0e0db96b6?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (bb9179a) will **decrease** coverage by `0.00%`.
> The diff coverage is `100.00%`.
[](https://codecov.io/gh/apache/celix/pull/376?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #376 +/- ##
==========================================
- Coverage 71.99% 71.98% -0.01%
==========================================
Files 219 219
Lines 35612 35593 -19
==========================================
- Hits 25640 25623 -17
+ Misses 9972 9970 -2
```
| [Impacted Files](https://codecov.io/gh/apache/celix/pull/376?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [libs/framework/src/bundle\_context.c](https://codecov.io/gh/apache/celix/pull/376/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-bGlicy9mcmFtZXdvcmsvc3JjL2J1bmRsZV9jb250ZXh0LmM=) | `76.31% <100.00%> (-0.47%)` | :arrow_down: |
| [...n\_websocket/v1/src/pubsub\_websocket\_topic\_sender.c](https://codecov.io/gh/apache/celix/pull/376/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YnVuZGxlcy9wdWJzdWIvcHVic3ViX2FkbWluX3dlYnNvY2tldC92MS9zcmMvcHVic3ViX3dlYnNvY2tldF90b3BpY19zZW5kZXIuYw==) | `84.35% <0.00%> (+1.11%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/celix/pull/376?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/celix/pull/376?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [bb9179a...f6af626](https://codecov.io/gh/apache/celix/pull/376?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
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
[GitHub] [celix] codecov-commenter commented on pull request #376: Eliminate unnecessary blocking and mutext from useServiceWithOptions.
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on pull request #376:
URL: https://github.com/apache/celix/pull/376#issuecomment-962397253
# [Codecov](https://codecov.io/gh/apache/celix/pull/376?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#376](https://codecov.io/gh/apache/celix/pull/376?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (2133824) into [master](https://codecov.io/gh/apache/celix/commit/bb9179a68fdadda8a4ad3cf79cb7ded0e0db96b6?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (bb9179a) will **decrease** coverage by `0.00%`.
> The diff coverage is `100.00%`.
> :exclamation: Current head 2133824 differs from pull request most recent head c070b8a. Consider uploading reports for the commit c070b8a to get more accurate results
[](https://codecov.io/gh/apache/celix/pull/376?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #376 +/- ##
==========================================
- Coverage 71.99% 71.99% -0.01%
==========================================
Files 219 219
Lines 35612 35593 -19
==========================================
- Hits 25640 25624 -16
+ Misses 9972 9969 -3
```
| [Impacted Files](https://codecov.io/gh/apache/celix/pull/376?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [libs/framework/src/bundle\_context.c](https://codecov.io/gh/apache/celix/pull/376/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-bGlicy9mcmFtZXdvcmsvc3JjL2J1bmRsZV9jb250ZXh0LmM=) | `76.31% <100.00%> (-0.47%)` | :arrow_down: |
| [.../pubsub\_admin\_tcp/v2/src/pubsub\_tcp\_topic\_sender.c](https://codecov.io/gh/apache/celix/pull/376/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YnVuZGxlcy9wdWJzdWIvcHVic3ViX2FkbWluX3RjcC92Mi9zcmMvcHVic3ViX3RjcF90b3BpY19zZW5kZXIuYw==) | `84.88% <0.00%> (-0.89%)` | :arrow_down: |
| [...n\_websocket/v1/src/pubsub\_websocket\_topic\_sender.c](https://codecov.io/gh/apache/celix/pull/376/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YnVuZGxlcy9wdWJzdWIvcHVic3ViX2FkbWluX3dlYnNvY2tldC92MS9zcmMvcHVic3ViX3dlYnNvY2tldF90b3BpY19zZW5kZXIuYw==) | `84.35% <0.00%> (+1.11%)` | :arrow_up: |
| [.../pubsub\_admin\_zmq/v2/src/pubsub\_zmq\_topic\_sender.c](https://codecov.io/gh/apache/celix/pull/376/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YnVuZGxlcy9wdWJzdWIvcHVic3ViX2FkbWluX3ptcS92Mi9zcmMvcHVic3ViX3ptcV90b3BpY19zZW5kZXIuYw==) | `88.18% <0.00%> (+1.26%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/celix/pull/376?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/celix/pull/376?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [bb9179a...c070b8a](https://codecov.io/gh/apache/celix/pull/376?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
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
[GitHub] [celix] pnoltes commented on pull request #376: Eliminate unnecessary blocking and mutext from useServiceWithOptions.
Posted by GitBox <gi...@apache.org>.
pnoltes commented on pull request #376:
URL: https://github.com/apache/celix/pull/376#issuecomment-974823133
I will leave this PR open a bit longer, so that other Celix committers can also review this.
--
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
[GitHub] [celix] PengZheng edited a comment on pull request #376: Eliminate unnecessary blocking and mutext from useServiceWithOptions.
Posted by GitBox <gi...@apache.org>.
PengZheng edited a comment 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.
PS: C version explanation is given in Chapter 11 of [Modern C](https://gustedt.gitlabpages.inria.fr/modern-c/), which is freely available. C++11 and C11 memory model is essentially the same thing, thus my argument above also applies to C.
--
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
[GitHub] [celix] PengZheng edited a comment on pull request #376: Eliminate unnecessary blocking and mutext from useServiceWithOptions.
Posted by GitBox <gi...@apache.org>.
PengZheng edited a comment 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.
PS: C version explanation is given in Chapter 19 of [Modern C](https://gustedt.gitlabpages.inria.fr/modern-c/), which is freely available. C++11 and C11 memory model is essentially the same thing, thus the above also applies to C.
--
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