You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@qpid.apache.org by lulf <gi...@git.apache.org> on 2016/08/23 18:43:05 UTC
[GitHub] qpid-dispatch pull request #96: Use atomic ops for ref_counts
GitHub user lulf opened a pull request:
https://github.com/apache/qpid-dispatch/pull/96
Use atomic ops for ref_counts
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/lulf/qpid-dispatch lulf/atomic-op-ref-counts
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/qpid-dispatch/pull/96.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #96
----
commit 88b04919bb3608cadc550164386f407154129cdc
Author: Ulf Lilleengen <lu...@redhat.com>
Date: 2016-08-23T11:47:17Z
DISPATCH-479 Use atomic ops for ref_counts
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] qpid-dispatch issue #96: Use atomic ops for ref_counts
Posted by alanconway <gi...@git.apache.org>.
Github user alanconway commented on the issue:
https://github.com/apache/qpid-dispatch/pull/96
I would wrap this (with inline functions or macros to avoid function call overhead) so that the impl details can be changed by platform. I think these ops are gcc specific, they definitely won't work on windows. Check the qpid-cpp codebase it has an atomic counter impl that uses GCC, windows or defaults to mutex. Its C++ but you can do something similar in C.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] qpid-dispatch pull request #96: Use atomic ops for ref_counts
Posted by grs <gi...@git.apache.org>.
Github user grs commented on a diff in the pull request:
https://github.com/apache/qpid-dispatch/pull/96#discussion_r75930357
--- Diff: src/connection_manager.c ---
@@ -254,9 +252,8 @@ qd_config_ssl_profile_t *qd_dispatch_configure_ssl_profile(qd_dispatch_t *qd, qd
ssl_profile->ssl_trusted_certificates = qd_entity_opt_string(entity, "trustedCerts", 0); CHECK();
ssl_profile->ssl_uid_format = qd_entity_opt_string(entity, "uidFormat", 0); CHECK();
ssl_profile->ssl_display_name_file = qd_entity_opt_string(entity, "displayNameFile", 0); CHECK();
- sys_mutex_lock(qd->connection_manager->ssl_profile_lock);
- ssl_profile->ref_count = 0;
- sys_mutex_unlock(qd->connection_manager->ssl_profile_lock);
+
+ __sync_and_and_fetch(&ssl_profile->ref_count, 0);
--- End diff --
Ah, I get it now! Sorry for the noise!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] qpid-dispatch issue #96: Use atomic ops for ref_counts
Posted by ted-ross <gi...@git.apache.org>.
Github user ted-ross commented on the issue:
https://github.com/apache/qpid-dispatch/pull/96
I like this change very much (I had planned to do it myself eventually). It would be better I think if rather than sprinkle the directives throughout the code, you could make a portable abstraction (similar to the threading module). Even if the abstraction is made up of macros, that would be ok. I assume that other compilers might use different syntax for atomic operations, and some might not support it at all and might need to revert to mutexes.
Perhaps there should be a portable ref_count abstraction with increment and decrement-test-for-zero operations.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] qpid-dispatch issue #96: Use atomic ops for ref_counts
Posted by lulf <gi...@git.apache.org>.
Github user lulf commented on the issue:
https://github.com/apache/qpid-dispatch/pull/96
@alanconway i think you might be commenting on an outdated diff. Look at the 'files changed' tab.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] qpid-dispatch issue #96: Use atomic ops for ref_counts
Posted by alanconway <gi...@git.apache.org>.
Github user alanconway commented on the issue:
https://github.com/apache/qpid-dispatch/pull/96
Yep - this looks great, I approve.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] qpid-dispatch pull request #96: Use atomic ops for ref_counts
Posted by lulf <gi...@git.apache.org>.
Github user lulf commented on a diff in the pull request:
https://github.com/apache/qpid-dispatch/pull/96#discussion_r75929783
--- Diff: src/connection_manager.c ---
@@ -254,9 +252,8 @@ qd_config_ssl_profile_t *qd_dispatch_configure_ssl_profile(qd_dispatch_t *qd, qd
ssl_profile->ssl_trusted_certificates = qd_entity_opt_string(entity, "trustedCerts", 0); CHECK();
ssl_profile->ssl_uid_format = qd_entity_opt_string(entity, "uidFormat", 0); CHECK();
ssl_profile->ssl_display_name_file = qd_entity_opt_string(entity, "displayNameFile", 0); CHECK();
- sys_mutex_lock(qd->connection_manager->ssl_profile_lock);
- ssl_profile->ref_count = 0;
- sys_mutex_unlock(qd->connection_manager->ssl_profile_lock);
+
+ __sync_and_and_fetch(&ssl_profile->ref_count, 0);
--- End diff --
No, i didn't find an atomic assign, but come to think of it, i think just regular assignment should work. This was just a workaround I did without thinking.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] qpid-dispatch pull request #96: Use atomic ops for ref_counts
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/qpid-dispatch/pull/96
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] qpid-dispatch issue #96: Use atomic ops for ref_counts
Posted by lulf <gi...@git.apache.org>.
Github user lulf commented on the issue:
https://github.com/apache/qpid-dispatch/pull/96
@ted-ross Good idea! I've updated the PR with a header for atomic operations that supports c11, gnu/clang builtins and uses mutexes as a fallback for other compilers. It might also be that the flags checked for GNUC and clang could be refined somewhat, but I think these builtins have been a part of those compilers for a long time. I don't have access to visual studio, so I haven't tested that.
It's not pretty, but I've tried to create as little overhead for this abstraction as possible. I wasn't sure if this should be in the public api or maybe in some internal header.
A generic ref_count/smart pointer abstraction can be built using this as the next step if desired.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] qpid-dispatch pull request #96: Use atomic ops for ref_counts
Posted by grs <gi...@git.apache.org>.
Github user grs commented on a diff in the pull request:
https://github.com/apache/qpid-dispatch/pull/96#discussion_r75927487
--- Diff: src/connection_manager.c ---
@@ -254,9 +252,8 @@ qd_config_ssl_profile_t *qd_dispatch_configure_ssl_profile(qd_dispatch_t *qd, qd
ssl_profile->ssl_trusted_certificates = qd_entity_opt_string(entity, "trustedCerts", 0); CHECK();
ssl_profile->ssl_uid_format = qd_entity_opt_string(entity, "uidFormat", 0); CHECK();
ssl_profile->ssl_display_name_file = qd_entity_opt_string(entity, "displayNameFile", 0); CHECK();
- sys_mutex_lock(qd->connection_manager->ssl_profile_lock);
- ssl_profile->ref_count = 0;
- sys_mutex_unlock(qd->connection_manager->ssl_profile_lock);
+
+ __sync_and_and_fetch(&ssl_profile->ref_count, 0);
--- End diff --
Typo? Should this be __sync_add_and_fetch?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org