You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@qpid.apache.org by GitBox <gi...@apache.org> on 2020/06/11 13:20:10 UTC

[GitHub] [qpid-dispatch] kgiusti opened a new pull request #760: DISPATCH-1679: Add sys_thread_self()

kgiusti opened a new pull request #760:
URL: https://github.com/apache/qpid-dispatch/pull/760


   sys_thread_self() provides an interface for accessing the sys_thread_t
   pointer for the currently running thread.  This replaces the old
   sys_thread_id() function which required the thread to already have its
   sys_thread_t pointer, which basically defeats the purpose.  In
   addition the return value of sys_thread_id() was pthread specific and
   should only be compared using pthread_equal().  sys_thread_self()
   return value is a plain pointer that can be compared in line.


----------------------------------------------------------------
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@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org

[GitHub] [qpid-dispatch] asfgit closed pull request #760: DISPATCH-1679: Add sys_thread_self()

Posted by GitBox <gi...@apache.org>.
asfgit closed pull request #760:
URL: https://github.com/apache/qpid-dispatch/pull/760


   


----------------------------------------------------------------
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@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org

[GitHub] [qpid-dispatch] jiridanek commented on pull request #760: DISPATCH-1679: Add sys_thread_self()

Posted by GitBox <gi...@apache.org>.
jiridanek commented on pull request #760:
URL: https://github.com/apache/qpid-dispatch/pull/760#issuecomment-642753722


   N.B. I am responsible for the death of previous incarnation of `sys_thread_self` in https://github.com/apache/qpid-dispatch/commit/3db622312ecaf6b872af01c258f4687fdb5d9720. The problem with previous implementation was that it assumed `pthread_self()` returns (typedef'd alias of) `long`, which is not always true. It's not true on macOS. Nobody was using that function then, so it quietly died.


----------------------------------------------------------------
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@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org

[GitHub] [qpid-dispatch] ChugR commented on a change in pull request #760: DISPATCH-1679: Add sys_thread_self()

Posted by GitBox <gi...@apache.org>.
ChugR commented on a change in pull request #760:
URL: https://github.com/apache/qpid-dispatch/pull/760#discussion_r438860724



##########
File path: src/posix/threading.c
##########
@@ -150,19 +150,38 @@ void sys_rwlock_unlock(sys_rwlock_t *lock)
 
 struct sys_thread_t {
     pthread_t thread;
+    void *(*f)(void *);
+    void *arg;
 };
 
+static __thread sys_thread_t *_self;
+
+
+// bootstrap _self before calling main thread function
+//
+static void *_thread_init(void *arg)
+{
+    _self = (sys_thread_t *)arg;
+    return _self->f(_self->arg);
+}
+
+
 sys_thread_t *sys_thread(void *(*run_function) (void *), void *arg)
 {
     sys_thread_t *thread = NEW(sys_thread_t);
-    pthread_create(&(thread->thread), 0, run_function, arg);
+    thread->f = run_function;
+    thread->arg = arg;
+    pthread_create(&(thread->thread), 0, _thread_init, (void *)thread);
     return thread;
 }
 
-long sys_thread_id(sys_thread_t *thread) {
-    return (long) thread->thread;
+

Review comment:
       Is sys_thread_id() now gone? Is the address returned by sys_thread_self now sufficient to be the "id"?




----------------------------------------------------------------
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@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org

[GitHub] [qpid-dispatch] ted-ross commented on a change in pull request #760: DISPATCH-1679: Add sys_thread_self()

Posted by GitBox <gi...@apache.org>.
ted-ross commented on a change in pull request #760:
URL: https://github.com/apache/qpid-dispatch/pull/760#discussion_r440209978



##########
File path: src/posix/threading.c
##########
@@ -150,19 +150,38 @@ void sys_rwlock_unlock(sys_rwlock_t *lock)
 
 struct sys_thread_t {
     pthread_t thread;
+    void *(*f)(void *);
+    void *arg;
 };
 
+static __thread sys_thread_t *_self;
+
+
+// bootstrap _self before calling main thread function
+//
+static void *_thread_init(void *arg)
+{
+    _self = (sys_thread_t *)arg;

Review comment:
       Style:
   `(sys_thread_t*) arg;`




----------------------------------------------------------------
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@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org

[GitHub] [qpid-dispatch] kgiusti commented on pull request #760: DISPATCH-1679: Add sys_thread_self()

Posted by GitBox <gi...@apache.org>.
kgiusti commented on pull request #760:
URL: https://github.com/apache/qpid-dispatch/pull/760#issuecomment-642861207


   > N.B. I am responsible for the death of previous incarnation of `sys_thread_self` in [3db6223](https://github.com/apache/qpid-dispatch/commit/3db622312ecaf6b872af01c258f4687fdb5d9720). The problem with previous implementation was that it assumed `pthread_self()` returns (typedef'd alias of) `long`, which is not always true. It's not true on macOS. Nobody was using that function then, so it quietly died.
   
   I'm hoping the new sys_thread_self() call will work on macOS as it avoids exporting the pthread_t type.


----------------------------------------------------------------
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@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org

[GitHub] [qpid-dispatch] kgiusti commented on a change in pull request #760: DISPATCH-1679: Add sys_thread_self()

Posted by GitBox <gi...@apache.org>.
kgiusti commented on a change in pull request #760:
URL: https://github.com/apache/qpid-dispatch/pull/760#discussion_r438992885



##########
File path: src/posix/threading.c
##########
@@ -150,19 +150,38 @@ void sys_rwlock_unlock(sys_rwlock_t *lock)
 
 struct sys_thread_t {
     pthread_t thread;
+    void *(*f)(void *);
+    void *arg;
 };
 
+static __thread sys_thread_t *_self;
+
+
+// bootstrap _self before calling main thread function
+//
+static void *_thread_init(void *arg)
+{
+    _self = (sys_thread_t *)arg;
+    return _self->f(_self->arg);
+}
+
+
 sys_thread_t *sys_thread(void *(*run_function) (void *), void *arg)
 {
     sys_thread_t *thread = NEW(sys_thread_t);
-    pthread_create(&(thread->thread), 0, run_function, arg);
+    thread->f = run_function;
+    thread->arg = arg;
+    pthread_create(&(thread->thread), 0, _thread_init, (void *)thread);
     return thread;
 }
 
-long sys_thread_id(sys_thread_t *thread) {
-    return (long) thread->thread;
+

Review comment:
       Yes and Yes!
   There were no calls to sys_thread_id() in the code... so zero risk in removing it.
   Each thread is allocated a unique sys_thread_t pointer that exists in shared memory so every thread's pointer will be globally unique.  One of the new unit tests verifies that is the case.




----------------------------------------------------------------
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@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org

[GitHub] [qpid-dispatch] ted-ross commented on a change in pull request #760: DISPATCH-1679: Add sys_thread_self()

Posted by GitBox <gi...@apache.org>.
ted-ross commented on a change in pull request #760:
URL: https://github.com/apache/qpid-dispatch/pull/760#discussion_r440210658



##########
File path: src/posix/threading.c
##########
@@ -150,19 +150,38 @@ void sys_rwlock_unlock(sys_rwlock_t *lock)
 
 struct sys_thread_t {
     pthread_t thread;
+    void *(*f)(void *);
+    void *arg;
 };
 
+static __thread sys_thread_t *_self;
+
+
+// bootstrap _self before calling main thread function
+//
+static void *_thread_init(void *arg)
+{
+    _self = (sys_thread_t *)arg;
+    return _self->f(_self->arg);
+}
+
+
 sys_thread_t *sys_thread(void *(*run_function) (void *), void *arg)
 {
     sys_thread_t *thread = NEW(sys_thread_t);
-    pthread_create(&(thread->thread), 0, run_function, arg);
+    thread->f = run_function;
+    thread->arg = arg;
+    pthread_create(&(thread->thread), 0, _thread_init, (void *)thread);

Review comment:
       Style:
   `(void*) thread`




----------------------------------------------------------------
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@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org

[GitHub] [qpid-dispatch] codecov-commenter commented on pull request #760: DISPATCH-1679: Add sys_thread_self()

Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on pull request #760:
URL: https://github.com/apache/qpid-dispatch/pull/760#issuecomment-644231061


   # [Codecov](https://codecov.io/gh/apache/qpid-dispatch/pull/760?src=pr&el=h1) Report
   > Merging [#760](https://codecov.io/gh/apache/qpid-dispatch/pull/760?src=pr&el=desc) into [master](https://codecov.io/gh/apache/qpid-dispatch/commit/57f778ac3bfed7b27cc8ee82f09e221ace4382be&el=desc) will **decrease** coverage by `0.01%`.
   > The diff coverage is `93.15%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/qpid-dispatch/pull/760/graphs/tree.svg?width=650&height=150&src=pr&token=rk2Cgd27pP)](https://codecov.io/gh/apache/qpid-dispatch/pull/760?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master     #760      +/-   ##
   ==========================================
   - Coverage   86.43%   86.42%   -0.02%     
   ==========================================
     Files          97       98       +1     
     Lines       22242    22312      +70     
   ==========================================
   + Hits        19225    19283      +58     
   - Misses       3017     3029      +12     
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/qpid-dispatch/pull/760?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [tests/thread\_test.c](https://codecov.io/gh/apache/qpid-dispatch/pull/760/diff?src=pr&el=tree#diff-dGVzdHMvdGhyZWFkX3Rlc3QuYw==) | `92.18% <92.18%> (ø)` | |
   | [src/posix/threading.c](https://codecov.io/gh/apache/qpid-dispatch/pull/760/diff?src=pr&el=tree#diff-c3JjL3Bvc2l4L3RocmVhZGluZy5j) | `95.23% <100.00%> (+3.85%)` | :arrow_up: |
   | [tests/run\_unit\_tests.c](https://codecov.io/gh/apache/qpid-dispatch/pull/760/diff?src=pr&el=tree#diff-dGVzdHMvcnVuX3VuaXRfdGVzdHMuYw==) | `78.57% <100.00%> (+0.79%)` | :arrow_up: |
   | [src/router\_core/connections.c](https://codecov.io/gh/apache/qpid-dispatch/pull/760/diff?src=pr&el=tree#diff-c3JjL3JvdXRlcl9jb3JlL2Nvbm5lY3Rpb25zLmM=) | `91.78% <0.00%> (-1.27%)` | :arrow_down: |
   | [src/router\_node.c](https://codecov.io/gh/apache/qpid-dispatch/pull/760/diff?src=pr&el=tree#diff-c3JjL3JvdXRlcl9ub2RlLmM=) | `93.25% <0.00%> (-0.11%)` | :arrow_down: |
   | [src/router\_core/transfer.c](https://codecov.io/gh/apache/qpid-dispatch/pull/760/diff?src=pr&el=tree#diff-c3JjL3JvdXRlcl9jb3JlL3RyYW5zZmVyLmM=) | `94.23% <0.00%> (+0.96%)` | :arrow_up: |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/qpid-dispatch/pull/760?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/qpid-dispatch/pull/760?src=pr&el=footer). Last update [57f778a...d9394e8](https://codecov.io/gh/apache/qpid-dispatch/pull/760?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   


----------------------------------------------------------------
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@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org