https://bugs.ruby-lang.org/https://bugs.ruby-lang.org/favicon.ico?17113305112017-05-10T00:17:25ZRuby Issue Tracking SystemRuby master - Feature #13552: [PATCH 0/2] reimplement ConditionVariable, Queue, SizedQueue using ccan/listhttps://bugs.ruby-lang.org/issues/13552?journal_id=647332017-05-10T00:17:25Znormalperson (Eric Wong)normalperson@yhbt.net
<ul><li><strong>File</strong> <a href="/attachments/6532">0002-thread_sync.c-rewrite-the-rest-using-using-ccan-list.patch</a> <a class="icon-only icon-download" title="Download" href="/attachments/download/6532/0002-thread_sync.c-rewrite-the-rest-using-using-ccan-list.patch">0002-thread_sync.c-rewrite-the-rest-using-using-ccan-list.patch</a> added</li></ul> Ruby master - Feature #13552: [PATCH 0/2] reimplement ConditionVariable, Queue, SizedQueue using ccan/listhttps://bugs.ruby-lang.org/issues/13552?journal_id=647342017-05-10T00:19:30Znormalperson (Eric Wong)normalperson@yhbt.net
<ul></ul><p>pull request:</p>
<p>The following changes since commit 6ad7c53ba9fb688ea1070a2319a64f0cc32c08e8:</p>
<p>test/thread: relax internal implementation check in error message (2017-05-09 19:52:10 +0000)</p>
<p>are available in the git repository at:</p>
<p>git://80x24.org/ruby.git sync-list</p>
<p>for you to fetch changes up to 4d77449e1c832d4398cdc07ef10b57e55bea1b81:</p>
<p>thread_sync.c: rewrite the rest using using ccan/list (2017-05-09 20:42:50 +0000)</p>
<hr>
<p>Eric Wong (2):<br>
thread_sync.c: rename mutex_waiter struct to sync_waiter<br>
thread_sync.c: rewrite the rest using using ccan/list</p>
<p>thread_sync.c | 487 ++++++++++++++++++++++++++++++++++++++--------------------<br>
1 file changed, 324 insertions(+), 163 deletions(-)</p> Ruby master - Feature #13552: [PATCH 0/2] reimplement ConditionVariable, Queue, SizedQueue using ccan/listhttps://bugs.ruby-lang.org/issues/13552?journal_id=648832017-05-18T17:21:27Znormalperson (Eric Wong)normalperson@yhbt.net
<ul></ul><p><a href="mailto:normalperson@yhbt.net" class="email">normalperson@yhbt.net</a> wrote:</p>
<blockquote>
<pre><code> thread_sync.c: rename mutex_waiter struct to sync_waiter
thread_sync.c: rewrite the rest using using ccan/list
</code></pre>
</blockquote>
<p>Any comment? Rebased patches against current trunk (r58783) available here:</p>
<pre><code>https://80x24.org/spew/20170516033841.1795-1-e@80x24.org/raw
https://80x24.org/spew/20170516033841.1795-2-e@80x24.org/raw
</code></pre>
<p>Thanks.</p>
<blockquote>
<p>Feature <a class="issue tracker-2 status-5 priority-4 priority-default closed" title="Feature: [PATCH 0/2] reimplement ConditionVariable, Queue, SizedQueue using ccan/list (Closed)" href="https://bugs.ruby-lang.org/issues/13552">#13552</a>: [PATCH 0/2] reimplement ConditionVariable, Queue, SizedQueue using ccan/list<br>
<a href="https://bugs.ruby-lang.org/issues/13552#change-64734" class="external">https://bugs.ruby-lang.org/issues/13552#change-64734</a></p>
</blockquote> Ruby master - Feature #13552: [PATCH 0/2] reimplement ConditionVariable, Queue, SizedQueue using ccan/listhttps://bugs.ruby-lang.org/issues/13552?journal_id=648912017-05-19T02:24:23Zko1 (Koichi Sasada)
<ul><li><strong>Status</strong> changed from <i>Open</i> to <i>Assigned</i></li><li><strong>Assignee</strong> set to <i>normalperson (Eric Wong)</i></li><li><strong>Target version</strong> set to <i>2.5</i></li></ul><p>Sorry for late response.</p>
<p>Only one comment (maybe you passes all of tests, right?)</p>
<p>New data type should be RUBY_TYPED_WB_PROTECTED (they need to use write barriers correctly).<br>
Do you want to try or should I modify?</p>
<p>Thanks,<br>
Koichi</p> Ruby master - Feature #13552: [PATCH 0/2] reimplement ConditionVariable, Queue, SizedQueue using ccan/listhttps://bugs.ruby-lang.org/issues/13552?journal_id=649002017-05-19T03:51:31Znormalperson (Eric Wong)normalperson@yhbt.net
<ul></ul><p><a href="mailto:ko1@atdot.net" class="email">ko1@atdot.net</a> wrote:</p>
<blockquote>
<p>Sorry for late response.</p>
</blockquote>
<p>No problem.</p>
<blockquote>
<p>Only one comment (maybe you passes all of tests, right?)</p>
</blockquote>
<p>Of course :)</p>
<blockquote>
<p>New data type should be RUBY_TYPED_WB_PROTECTED (they need to use write barriers correctly).<br>
Do you want to try or should I modify?</p>
</blockquote>
<p>I'm still not very familiar with RGenGC, but here is my try:</p>
<pre><code>https://80x24.org/spew/20170519034419.GA29820@whir/raw
</code></pre>
<p>I'm not sure how this helps performance, however. The Arrays<br>
are constantly changing with push/pop and RGenGC works best for<br>
stable (unchanging) objects (correct?)</p>
<p>Also, does setting RUBY_TYPED_WB_PROTECTED make sense for<br>
rb_condvar and rb_mutex_t? They store no Ruby objects and<br>
have no dmark callback.</p>
<p>Thanks.</p> Ruby master - Feature #13552: [PATCH 0/2] reimplement ConditionVariable, Queue, SizedQueue using ccan/listhttps://bugs.ruby-lang.org/issues/13552?journal_id=649082017-05-19T06:39:36Zko1 (Koichi Sasada)
<ul></ul><blockquote>
<p><a href="https://80x24.org/spew/20170519034419.GA29820@whir/raw" class="external">https://80x24.org/spew/20170519034419.GA29820@whir/raw</a></p>
</blockquote>
<p>Thank you. Adding <code>const</code> helps us to recognize.</p>
<pre><code>PACKED_STRUCT_UNALIGNED(struct rb_queue {
struct list_head waitq;
const VALUE que;
int num_waiting;
});
</code></pre>
<blockquote>
<p>I'm not sure how this helps performance, however. The Arrays<br>
are constantly changing with push/pop and RGenGC works best for<br>
stable (unchanging) objects (correct?)</p>
</blockquote>
<p>Sorry, I can't understand your question.<br>
Could you give me your question in other words?</p>
<blockquote>
<p>Also, does setting RUBY_TYPED_WB_PROTECTED make sense for<br>
rb_condvar and rb_mutex_t? They store no Ruby objects and<br>
have no dmark callback.</p>
</blockquote>
<p>Yes, please. not wb protected objects become roots for all of minor gc.<br>
No write is the best wb protected object.</p> Ruby master - Feature #13552: [PATCH 0/2] reimplement ConditionVariable, Queue, SizedQueue using ccan/listhttps://bugs.ruby-lang.org/issues/13552?journal_id=649312017-05-19T08:11:35Znormalperson (Eric Wong)normalperson@yhbt.net
<ul></ul><p><a href="mailto:ko1@atdot.net" class="email">ko1@atdot.net</a> wrote:</p>
<blockquote>
<blockquote>
<p><a href="https://80x24.org/spew/20170519034419.GA29820@whir/raw" class="external">https://80x24.org/spew/20170519034419.GA29820@whir/raw</a></p>
</blockquote>
<p>Thank you. Adding <code>const</code> helps us to recognize.</p>
<pre><code>PACKED_STRUCT_UNALIGNED(struct rb_queue {
struct list_head waitq;
const VALUE que;
int num_waiting;
});
</code></pre>
</blockquote>
<p>Thank you for that advice! I will update tomorrow.</p>
<blockquote>
<pre><code>
> I'm not sure how this helps performance, however. The Arrays
> are constantly changing with push/pop and RGenGC works best for
> stable (unchanging) objects (correct?)
Sorry, I can't understand your question.
Could you give me your question in other words?
</code></pre>
</blockquote>
<p>Generational GC tries to avoid marking since "old" generation<br>
does not change references.</p>
<p>However, the ->que in Queue/SizedQueue is always changing<br>
because threads push/pop. When references are always changing<br>
in Queues, so GC needs mark ->que frequently.</p>
<blockquote>
<blockquote>
<p>Also, does setting RUBY_TYPED_WB_PROTECTED make sense for<br>
rb_condvar and rb_mutex_t? They store no Ruby objects and<br>
have no dmark callback.</p>
</blockquote>
<p>Yes, please. not wb protected objects become roots for all of minor gc.<br>
No write is the best wb protected object.</p>
</blockquote>
<p>Good to know! I will update and commit tomorrow.</p> Ruby master - Feature #13552: [PATCH 0/2] reimplement ConditionVariable, Queue, SizedQueue using ccan/listhttps://bugs.ruby-lang.org/issues/13552?journal_id=649622017-05-19T18:53:17ZAnonymous
<ul><li><strong>Status</strong> changed from <i>Assigned</i> to <i>Closed</i></li></ul><p>Applied in changeset trunk|r58805.</p>
<hr>
<p>thread_sync.c: rewrite the rest using using ccan/list</p>
<p>The performance improvement increases as the number of waiters<br>
increases, due to avoiding the O(n) behavior of rb_ary_delete on<br>
the waiting thread. Uncontended queues and condition variables<br>
performance is not altered significantly.</p>
<p>Function entry cost is slightly increased for ConditionVariable,<br>
since the data pointer is separately allocated and not embedded<br>
into the RVALUE slot.</p>
<p><a href="/issues/13552">[ruby-core:81235]</a> [Feature <a class="issue tracker-2 status-5 priority-4 priority-default closed" title="Feature: [PATCH 0/2] reimplement ConditionVariable, Queue, SizedQueue using ccan/list (Closed)" href="https://bugs.ruby-lang.org/issues/13552">#13552</a>]</p>
<table>
<thead>
<tr>
<th>name</th>
<th align="right">trunk</th>
<th align="right">built</th>
</tr>
</thead>
<tbody>
<tr>
<td>vm_thread_condvar1</td>
<td align="right">0.858</td>
<td align="right">0.858</td>
</tr>
<tr>
<td>vm_thread_condvar2</td>
<td align="right">1.003</td>
<td align="right">0.804</td>
</tr>
<tr>
<td>vm_thread_queue</td>
<td align="right">0.131</td>
<td align="right">0.129</td>
</tr>
<tr>
<td>vm_thread_sized_queue</td>
<td align="right">0.265</td>
<td align="right">0.251</td>
</tr>
<tr>
<td>vm_thread_sized_queue2</td>
<td align="right">0.892</td>
<td align="right">0.859</td>
</tr>
<tr>
<td>vm_thread_sized_queue3</td>
<td align="right">0.879</td>
<td align="right">0.845</td>
</tr>
<tr>
<td>vm_thread_sized_queue4</td>
<td align="right">0.599</td>
<td align="right">0.486</td>
</tr>
</tbody>
</table>
<p>Speedup ratio: compare with the result of `trunk' (greater is better)</p>
<table>
<thead>
<tr>
<th>name</th>
<th align="right">built</th>
</tr>
</thead>
<tbody>
<tr>
<td>vm_thread_condvar1</td>
<td align="right">0.999</td>
</tr>
<tr>
<td>vm_thread_condvar2</td>
<td align="right">1.246</td>
</tr>
<tr>
<td>vm_thread_queue</td>
<td align="right">1.020</td>
</tr>
<tr>
<td>vm_thread_sized_queue</td>
<td align="right">1.057</td>
</tr>
<tr>
<td>vm_thread_sized_queue2</td>
<td align="right">1.039</td>
</tr>
<tr>
<td>vm_thread_sized_queue3</td>
<td align="right">1.041</td>
</tr>
<tr>
<td>vm_thread_sized_queue4</td>
<td align="right">1.233</td>
</tr>
</tbody>
</table>