https://bugs.ruby-lang.org/https://bugs.ruby-lang.org/favicon.ico?17113305112013-05-20T10:23:17ZRuby Issue Tracking SystemRuby master - Feature #8426: Implement class hierarchy method cachinghttps://bugs.ruby-lang.org/issues/8426?journal_id=394442013-05-20T10:23:17Zduerst (Martin Dürst)duerst@it.aoyama.ac.jp
<ul></ul><p>Hello Charlie,</p>
<p>This sounds very promising, as it should make Ruby faster. Any idea how<br>
much faster? And are there cases where it might be slower, or other<br>
disadvantages?</p>
<p>Regards, Martin.</p>
<p>On 2013/05/19 19:44, charliesome (Charlie Somerville) wrote:</p>
<blockquote>
<p>Issue <a class="issue tracker-2 status-5 priority-4 priority-default closed" title="Feature: Implement class hierarchy method caching (Closed)" href="https://bugs.ruby-lang.org/issues/8426">#8426</a> has been reported by charliesome (Charlie Somerville).</p>
<hr>
<p>Feature <a class="issue tracker-2 status-5 priority-4 priority-default closed" title="Feature: Implement class hierarchy method caching (Closed)" href="https://bugs.ruby-lang.org/issues/8426">#8426</a>: Implement class hierarchy method caching<br>
<a href="https://bugs.ruby-lang.org/issues/8426" class="external">https://bugs.ruby-lang.org/issues/8426</a></p>
<p>Author: charliesome (Charlie Somerville)<br>
Status: Open<br>
Priority: Normal<br>
Assignee:<br>
Category:<br>
Target version:</p>
<p>=begin<br>
This patch adds class hierarchy method caching to CRuby. This is the algorithm used by JRuby and Rubinius.</p>
</blockquote> Ruby master - Feature #8426: Implement class hierarchy method cachinghttps://bugs.ruby-lang.org/issues/8426?journal_id=394472013-05-20T11:22:45Zsam.saffron (Sam Saffron)sam.saffron@gmail.com
<ul></ul><p>Here are some raw benches comparing Ruby-Head with KclassCache</p>
<p>TLDR;</p>
<p>Noticeable improvement over head.</p>
<p>Discourse topic list page: 69 median -> 65 median , 78.3 mean -> 67.4 mean<br>
Discourse topic page: 51 median -> 48 median , 57 mean -> 50 mean</p>
<p>HEAD</p>
<p>sam@ubuntu:~/Source/discourse$ ab -n 200 <a href="http://l.discourse/t/quote-reply-gets-in-the-way/1495" class="external">http://l.discourse/t/quote-reply-gets-in-the-way/1495</a><br>
This is ApacheBench, Version 2.3 <$Revision: 655654 $><br>
Copyright 1996 Adam Twiss, Zeus Technology Ltd, <a href="http://www.zeustech.net/" class="external">http://www.zeustech.net/</a><br>
Licensed to The Apache Software Foundation, <a href="http://www.apache.org/" class="external">http://www.apache.org/</a></p>
<p>Benchmarking l.discourse (be patient)<br>
Completed 100 requests<br>
Completed 200 requests<br>
Finished 200 requests</p>
<p>Server Software: nginx/1.2.6<br>
Server Hostname: l.discourse<br>
Server Port: 80</p>
<p>Document Path: /t/quote-reply-gets-in-the-way/1495<br>
Document Length: 54925 bytes</p>
<p>Concurrency Level: 1<br>
Time taken for tests: 11.406 seconds<br>
Complete requests: 200<br>
Failed requests: 0<br>
Write errors: 0<br>
Total transferred: 11059400 bytes<br>
HTML transferred: 10985000 bytes<br>
Requests per second: 17.53 [#/sec] (mean)<br>
Time per request: 57.032 [ms] (mean)<br>
Time per request: 57.032 [ms] (mean, across all concurrent requests)<br>
Transfer rate: 946.86 [Kbytes/sec] received</p>
<p>Connection Times (ms)<br>
min mean[+/-sd] median max<br>
Connect: 0 0 0.0 0 0<br>
Processing: 49 57 23.4 50 184<br>
Waiting: 49 57 23.4 50 184<br>
Total: 49 57 23.4 51 184</p>
<p>Percentage of the requests served within a certain time (ms)<br>
50% 51<br>
66% 52<br>
75% 53<br>
80% 54<br>
90% 59<br>
95% 82<br>
98% 166<br>
99% 174<br>
100% 184 (longest request)</p>
<p>sam@ubuntu:~/Source/discourse$ ab -n 200 <a href="http://l.discourse/" class="external">http://l.discourse/</a><br>
This is ApacheBench, Version 2.3 <$Revision: 655654 $><br>
Copyright 1996 Adam Twiss, Zeus Technology Ltd, <a href="http://www.zeustech.net/" class="external">http://www.zeustech.net/</a><br>
Licensed to The Apache Software Foundation, <a href="http://www.apache.org/" class="external">http://www.apache.org/</a></p>
<p>Benchmarking l.discourse (be patient)<br>
Completed 100 requests<br>
Completed 200 requests<br>
Finished 200 requests</p>
<p>Server Software: nginx/1.2.6<br>
Server Hostname: l.discourse<br>
Server Port: 80</p>
<p>Document Path: /<br>
Document Length: 44604 bytes</p>
<p>Concurrency Level: 1<br>
Time taken for tests: 15.667 seconds<br>
Complete requests: 200<br>
Failed requests: 0<br>
Write errors: 0<br>
Total transferred: 8986000 bytes<br>
HTML transferred: 8920800 bytes<br>
Requests per second: 12.77 [#/sec] (mean)<br>
Time per request: 78.335 [ms] (mean)<br>
Time per request: 78.335 [ms] (mean, across all concurrent requests)<br>
Transfer rate: 560.12 [Kbytes/sec] received</p>
<p>Connection Times (ms)<br>
min mean[+/-sd] median max<br>
Connect: 0 0 0.0 0 0<br>
Processing: 67 78 33.8 69 232<br>
Waiting: 67 78 33.8 68 232<br>
Total: 67 78 33.8 69 232</p>
<p>Percentage of the requests served within a certain time (ms)<br>
50% 69<br>
66% 69<br>
75% 69<br>
80% 70<br>
90% 73<br>
95% 205<br>
98% 210<br>
99% 212<br>
100% 232 (longest request)<br>
sam@ubuntu:~/Source/discourse$</p>
<p>KCLASS_CACHE</p>
<p>sam@ubuntu:~/Source/discourse$ ab -n 200 <a href="http://l.discourse/t/quote-reply-gets-in-the-way/1495" class="external">http://l.discourse/t/quote-reply-gets-in-the-way/1495</a><br>
This is ApacheBench, Version 2.3 <$Revision: 655654 $><br>
Copyright 1996 Adam Twiss, Zeus Technology Ltd, <a href="http://www.zeustech.net/" class="external">http://www.zeustech.net/</a><br>
Licensed to The Apache Software Foundation, <a href="http://www.apache.org/" class="external">http://www.apache.org/</a></p>
<p>Benchmarking l.discourse (be patient)<br>
Completed 100 requests<br>
Completed 200 requests<br>
Finished 200 requests</p>
<p>Server Software: nginx/1.2.6<br>
Server Hostname: l.discourse<br>
Server Port: 80</p>
<p>Document Path: /t/quote-reply-gets-in-the-way/1495<br>
Document Length: 54925 bytes</p>
<p>Concurrency Level: 1<br>
Time taken for tests: 10.010 seconds<br>
Complete requests: 200<br>
Failed requests: 0<br>
Write errors: 0<br>
Total transferred: 11059400 bytes<br>
HTML transferred: 10985000 bytes<br>
Requests per second: 19.98 [#/sec] (mean)<br>
Time per request: 50.049 [ms] (mean)<br>
Time per request: 50.049 [ms] (mean, across all concurrent requests)<br>
Transfer rate: 1078.97 [Kbytes/sec] received</p>
<p>Connection Times (ms)<br>
min mean[+/-sd] median max<br>
Connect: 0 0 0.0 0 0<br>
Processing: 45 50 15.1 48 227<br>
Waiting: 45 50 15.1 47 226<br>
Total: 45 50 15.1 48 227</p>
<p>Percentage of the requests served within a certain time (ms)<br>
50% 48<br>
66% 48<br>
75% 48<br>
80% 48<br>
90% 49<br>
95% 70<br>
98% 99<br>
99% 101<br>
100% 227 (longest request)<br>
sam@ubuntu:~/Source/discourse$</p>
<p>sam@ubuntu:~/Source/discourse$ ab -n 200 <a href="http://l.discourse/" class="external">http://l.discourse/</a><br>
This is ApacheBench, Version 2.3 <$Revision: 655654 $><br>
Copyright 1996 Adam Twiss, Zeus Technology Ltd, <a href="http://www.zeustech.net/" class="external">http://www.zeustech.net/</a><br>
Licensed to The Apache Software Foundation, <a href="http://www.apache.org/" class="external">http://www.apache.org/</a></p>
<p>Benchmarking l.discourse (be patient)<br>
Completed 100 requests<br>
Completed 200 requests<br>
Finished 200 requests</p>
<p>Server Software: nginx/1.2.6<br>
Server Hostname: l.discourse<br>
Server Port: 80</p>
<p>Document Path: /<br>
Document Length: 44604 bytes</p>
<p>Concurrency Level: 1<br>
Time taken for tests: 13.480 seconds<br>
Complete requests: 200<br>
Failed requests: 0<br>
Write errors: 0<br>
Total transferred: 8986000 bytes<br>
HTML transferred: 8920800 bytes<br>
Requests per second: 14.84 [#/sec] (mean)<br>
Time per request: 67.403 [ms] (mean)<br>
Time per request: 67.403 [ms] (mean, across all concurrent requests)<br>
Transfer rate: 650.97 [Kbytes/sec] received</p>
<p>Connection Times (ms)<br>
min mean[+/-sd] median max<br>
Connect: 0 0 0.0 0 0<br>
Processing: 62 67 14.5 65 225<br>
Waiting: 62 67 14.5 65 225<br>
Total: 62 67 14.5 65 225</p>
<p>Percentage of the requests served within a certain time (ms)<br>
50% 65<br>
66% 65<br>
75% 66<br>
80% 66<br>
90% 67<br>
95% 86<br>
98% 115<br>
99% 115<br>
100% 225 (longest request)<br>
sam@ubuntu:~/Source/discourse$</p> Ruby master - Feature #8426: Implement class hierarchy method cachinghttps://bugs.ruby-lang.org/issues/8426?journal_id=394482013-05-20T12:53:23Zko1 (Koichi Sasada)
<ul></ul><p>Great work!</p>
<p>Could you explain the data stracture? Patch seems to introduce new data<br>
structure `sparse array'. What is this and how to use it on this patch?</p>
<p>And another consern is verification mechanism of the result. Complex<br>
methoc caching mechanism introduces bugs because:</p>
<ul>
<li>Everyone make bugs.</li>
<li>If someone who doesn't care method cache mechanism adds new<br>
core feature such as refinement and so on, it will break assumption<br>
about method caching.<br>
And this bug is difficult to find out because they may be rare.</li>
</ul>
<p>My proposal is to add verify mode (on/off by macro, of course off as<br>
default) which check the cached result using a naive method search.</p>
<p>#define verify 0<br>
result = ...<br>
#if verify<br>
if (naive_method_search() != result) rb_bug(...);<br>
#endif</p>
<p>It will help debugging.</p>
<a name="minor-comment-sa_-prefix-is-too-short-P"></a>
<h1 >minor comment: `sa_' prefix is too short :P<a href="#minor-comment-sa_-prefix-is-too-short-P" class="wiki-anchor">¶</a></h1>
<a name="minor-comment-change-of-extextmkrb-seems-not-needed"></a>
<h1 >minor comment: change of ext/extmk.rb seems not needed<a href="#minor-comment-change-of-extextmkrb-seems-not-needed" class="wiki-anchor">¶</a></h1>
<p><a href="https://github.com/charliesome/ruby/compare/trunk...klasscache-trunk#L4L219" class="external">https://github.com/charliesome/ruby/compare/trunk...klasscache-trunk#L4L219</a></p>
<a name="minor-comment-using-uint64_t-directly-is-not-preferable"></a>
<h1 >minor comment: using uint64_t directly is not preferable.<a href="#minor-comment-using-uint64_t-directly-is-not-preferable" class="wiki-anchor">¶</a></h1>
<p>for example:<br>
#if HAVE_UINT64_T<br>
typedef version_t uint64_t;<br>
#else<br>
typedef version_t uint_t;<br>
#endif</p>
<p>(2013/05/19 19:44), charliesome (Charlie Somerville) wrote:</p>
<blockquote>
<p>Issue <a class="issue tracker-2 status-5 priority-4 priority-default closed" title="Feature: Implement class hierarchy method caching (Closed)" href="https://bugs.ruby-lang.org/issues/8426">#8426</a> has been reported by charliesome (Charlie Somerville).</p>
<hr>
<p>Feature <a class="issue tracker-2 status-5 priority-4 priority-default closed" title="Feature: Implement class hierarchy method caching (Closed)" href="https://bugs.ruby-lang.org/issues/8426">#8426</a>: Implement class hierarchy method caching<br>
<a href="https://bugs.ruby-lang.org/issues/8426" class="external">https://bugs.ruby-lang.org/issues/8426</a></p>
<p>Author: charliesome (Charlie Somerville)<br>
Status: Open<br>
Priority: Normal<br>
Assignee:<br>
Category:<br>
Target version:</p>
<p>=begin<br>
This patch adds class hierarchy method caching to CRuby. This is the algorithm used by JRuby and Rubinius.</p>
<p>Currently, Ruby's method caches can only be expired globally. This means libraries that dynamically define methods or extend objects at runtime (eg. OpenStruct) can cause quite a significant performance hit.</p>
<p>With this patch, each class carries a monotonically increasing sequence number. Whenever an operation which would ordinarily cause a global method cache invalidation is performed, the sequence number on the affected class and all subclasses (classes hold weak references to their subclasses) is incremented, invalidating only method caches for those classes.</p>
<p>In this patch I've also split the (({getconstant})) VM instruction into two separate instructions - (({getclassconstant})) and (({getcrefconstant})). It's hoped that (({getclassconstant})) can start using class hierarchy caching with not much more effort. This change does affect compatibility in a minor way. Without this patch, (({nil::SomeConstant})) will look up (({SomeConstant})) in the current scope in CRuby (but not JRuby or Rubinius). With this patch, (({nil::SomeConstant})) will raise an exception.</p>
<p>The patch and all its commits can be viewed here: <a href="https://github.com/charliesome/ruby/compare/trunk...klasscache-trunk" class="external">https://github.com/charliesome/ruby/compare/trunk...klasscache-trunk</a></p>
<p>Big thanks to James Golick, who originally wrote this patch for Ruby 1.9.3.<br>
=end</p>
</blockquote>
<p>--<br>
// SASADA Koichi at atdot dot net</p> Ruby master - Feature #8426: Implement class hierarchy method cachinghttps://bugs.ruby-lang.org/issues/8426?journal_id=394502013-05-20T16:23:31Zfunny_falcon (Yura Sokolov)funny.falcon@gmail.com
<ul></ul><p>Good day, Koichi</p>
<p>"sparse array" - is a lightweight hash structure which maps 32bit integers to st_data_t values.<br>
It is more compact and faster replacement for st_table for integers (aka st_init_numtable).<br>
It is CPU cache friendly on read, and it's hash function is tuned against ID pattern<br>
(tuned is a great word, I were just lucky. At least, every other "better" hash function,<br>
like MurmurHash3 finalization, produce worse overall performance, and I could not explain why).</p>
<p>I've made it as a replacement for all usages of st_table as symbol table in my patch: methods,<br>
constants, ivars, - and it shows noticeable performance gain (~5-8%). When James Golick makes<br>
its method caching patch, I recommend him to use "sparse array", and he reports it efficiency.</p>
<p>It will be even better to embed sa_table into rb_classext_struct and do not allocate it separately.<br>
If patch will be accepted, I could made such change.</p>
<p>Considering uint64_t - it should be 64bit value, so that there is no need to check for overflow<br>
(even if one increments it 4_000_000_000 per second, it will take 70 years to overflow).<br>
So that, it should be</p>
<p>#if HAVE_UINT64_T<br>
typedef uint64_t version_t;<br>
#else<br>
typedef long long version_t ;<br>
#endif</p> Ruby master - Feature #8426: Implement class hierarchy method cachinghttps://bugs.ruby-lang.org/issues/8426?journal_id=394512013-05-20T16:28:24Zfunny_falcon (Yura Sokolov)funny.falcon@gmail.com
<ul></ul><p>Charlie, why sa_index_t is uint64_t ? it really should be 32bit for better CPU cache locality.<br>
Yes, it will limits ID to 32bit values, but ID should not increase to greater values,<br>
otherwise it is a memory leak.</p> Ruby master - Feature #8426: Implement class hierarchy method cachinghttps://bugs.ruby-lang.org/issues/8426?journal_id=394532013-05-20T18:23:17ZAnonymous
<ul></ul><p>On Monday, 20 May 2013 at 5:28 PM, funny_falcon (Yura Sokolov) wrote:</p>
<blockquote>
<p>Charlie, why sa_index_t is uint64_t ? it really should be 32bit for better CPU cache locality.<br>
Yes, it will limits ID to 32bit values, but ID should not increase to greater values,<br>
otherwise it is a memory leak.<br>
Sorry, this was an oversight. I've pushed a commit to make sa_index_t 32 bit.</p>
</blockquote> Ruby master - Feature #8426: Implement class hierarchy method cachinghttps://bugs.ruby-lang.org/issues/8426?journal_id=394542013-05-20T18:23:17ZAnonymous
<ul></ul><p>On Monday, 20 May 2013 at 1:35 PM, SASADA Koichi wrote:</p>
<blockquote>
<p>Could you explain the data stracture? Patch seems to introduce new data<br>
structure `sparse array'. What is this and how to use it on this patch?</p>
</blockquote>
<p>funny_falcon explained this well. It's significantly faster in this case when compared to st_table.</p>
<blockquote>
<p>And another consern is verification mechanism of the result. Complex<br>
methoc caching mechanism introduces bugs because:</p>
<ul>
<li>Everyone make bugs.</li>
<li>If someone who doesn't care method cache mechanism adds new<br>
core feature such as refinement and so on, it will break assumption<br>
about method caching.<br>
And this bug is difficult to find out because they may be rare.</li>
</ul>
<p>My proposal is to add verify mode (on/off by macro, of course off as<br>
default) which check the cached result using a naive method search.</p>
<p>#define verify 0<br>
result = ...<br>
#if verify<br>
if (naive_method_search() != result) rb_bug(...);<br>
#endif</p>
<p>It will help debugging.<br>
I think this is a reasonable proposal. I'll add it.</p>
</blockquote>
<blockquote>
<a name="minor-comment-sa_-prefix-is-too-short-P"></a>
<h1 >minor comment: `sa_' prefix is too short :P<a href="#minor-comment-sa_-prefix-is-too-short-P" class="wiki-anchor">¶</a></h1>
</blockquote>
<p>What would you suggest? Ruby already exports symbols with short prefixes, eg. st_.</p>
<blockquote>
<a name="minor-comment-change-of-extextmkrb-seems-not-needed"></a>
<h1 >minor comment: change of ext/extmk.rb seems not needed<a href="#minor-comment-change-of-extextmkrb-seems-not-needed" class="wiki-anchor">¶</a></h1>
<p><a href="https://github.com/charliesome/ruby/compare/trunk...klasscache-trunk#L4L219" class="external">https://github.com/charliesome/ruby/compare/trunk...klasscache-trunk#L4L219</a></p>
</blockquote>
<p>Whoops, fixed! Thanks for pointing this out.</p>
<blockquote>
<a name="minor-comment-using-uint64_t-directly-is-not-preferable"></a>
<h1 >minor comment: using uint64_t directly is not preferable.<a href="#minor-comment-using-uint64_t-directly-is-not-preferable" class="wiki-anchor">¶</a></h1>
<p>for example:<br>
#if HAVE_UINT64_T<br>
typedef version_t uint64_t;<br>
#else<br>
typedef version_t uint_t;<br>
#endif</p>
</blockquote>
<p>This is also a reasonable suggestion. I have introduced a new vm_state_version_t typedef.</p>
<p>Thanks for your feedback!</p> Ruby master - Feature #8426: Implement class hierarchy method cachinghttps://bugs.ruby-lang.org/issues/8426?journal_id=394552013-05-20T18:29:10Zko1 (Koichi Sasada)
<ul></ul><p>(2013/05/20 16:23), funny_falcon (Yura Sokolov) wrote:</p>
<blockquote>
<p>"sparse array" - is a lightweight hash structure which maps 32bit integers to st_data_t values.<br>
It is more compact and faster replacement for st_table for integers (aka st_init_numtable).<br>
It is CPU cache friendly on read, and it's hash function is tuned against ID pattern<br>
(tuned is a great word, I were just lucky. At least, every other "better" hash function,<br>
like MurmurHash3 finalization, produce worse overall performance, and I could not explain why).</p>
<p>I've made it as a replacement for all usages of st_table as symbol table in my patch: methods,<br>
constants, ivars, - and it shows noticeable performance gain (~5-8%). When James Golick makes<br>
its method caching patch, I recommend him to use "sparse array", and he reports it efficiency.</p>
<p>It will be even better to embed sa_table into rb_classext_struct and do not allocate it separately.<br>
If patch will be accepted, I could made such change.</p>
</blockquote>
<p>I got it (I don't check data strucuture details).</p>
<p>I prefer that it is similar name with st, for example, st_numtable_t, I<br>
can associate with special case of `table'. But not strong opinion.</p>
<p>If st_init_numtable() returns st_table * but use sa.c functions, it<br>
seems cool (OO-way). but additional branch cost (so high?).</p>
<blockquote>
<p>Considering uint64_t - it should be 64bit value, so that there is no need to check for overflow<br>
(even if one increments it 4_000_000_000 per second, it will take 70 years to overflow).<br>
So that, it should be</p>
<p>#if HAVE_UINT64_T<br>
typedef uint64_t version_t;<br>
#else<br>
typedef long long version_t ;<br>
#endif</p>
</blockquote>
<p>I understand your concern. My last suspicious is that I'm not sure `long<br>
long' is always supported. however, i'm not sure there is such<br>
environment, too. there is a similar discussion (we can assume 64bit<br>
integer type or not). Experts may dicide it.</p>
<p>--<br>
// SASADA Koichi at atdot dot net</p> Ruby master - Feature #8426: Implement class hierarchy method cachinghttps://bugs.ruby-lang.org/issues/8426?journal_id=394562013-05-20T18:53:10Zko1 (Koichi Sasada)
<ul></ul><p>(2013/05/20 18:21), Charlie Somerville wrote:</p>
<blockquote>
<p>funny_falcon explained this well. It's significantly faster in this case<br>
when compared to st_table.</p>
</blockquote>
<p>Thanks guys, I understand. Maybe it is used to implement weak reference<br>
from super class to sub classes, right?</p>
<blockquote>
<blockquote>
<p>It will help debugging.<br>
I think this is a reasonable proposal. I'll add it.</p>
</blockquote>
</blockquote>
<p>Thanks.</p>
<blockquote>
<blockquote>
<a name="minor-comment-sa_-prefix-is-too-short-P"></a>
<h1 >minor comment: `sa_' prefix is too short :P<a href="#minor-comment-sa_-prefix-is-too-short-P" class="wiki-anchor">¶</a></h1>
</blockquote>
<p>What would you suggest? Ruby already exports symbols with short<br>
prefixes, eg. st_.</p>
</blockquote>
<p>I prefer `st_' related name. But not strong opinion.</p>
<p>One more:</p>
<pre><code> if (LIKELY(GET_METHOD_STATE_VERSION() == ci->vmstat &&
RCLASS_EXT(klass)->seq == ci->seq &&
klass == ci->klass)) {
</code></pre>
<p>should be:</p>
<pre><code> if (LIKELY(GET_METHOD_STATE_VERSION() == ci->vmstat &&
klass == ci->klass &&
RCLASS_EXT(klass)->seq == ci->seq) {
</code></pre>
<p>...?<br>
why you use vmstat?</p>
<pre><code> if (klass == ci->klass &&
RCLASS_EXT(klass)->seq == ci->seq) {
</code></pre>
<p>is not enough?</p>
<p>Ah, you only use for re-def BasicObject, Object and Kernel.</p>
<ul>
<li>if (klass == rb_cBasicObject || klass == rb_cObject || klass ==<br>
rb_mKernel) {</li>
<li>
<pre><code> INC_METHOD_STATE_VERSION();
</code></pre>
</li>
<li>} else {</li>
</ul>
<p>Is it huge performance bottleneck? I think branch on inline cache should<br>
be removed.</p>
<p>--<br>
// SASADA Koichi at atdot dot net</p> Ruby master - Feature #8426: Implement class hierarchy method cachinghttps://bugs.ruby-lang.org/issues/8426?journal_id=394572013-05-20T19:10:21Zfunny_falcon (Yura Sokolov)funny.falcon@gmail.com
<ul></ul><p>ko1 (Koichi Sasada) wrote:</p>
<blockquote>
<p>(2013/05/20 18:21), Charlie Somerville wrote:</p>
<blockquote>
<p>funny_falcon explained this well. It's significantly faster in this case<br>
when compared to st_table.</p>
</blockquote>
<p>Thanks guys, I understand. Maybe it is used to implement weak reference<br>
from super class to sub classes, right?</p>
</blockquote>
<p>"sparse array" uses 32bit keys for being as small and CPU cache friendly as possible.<br>
So that, it could not store 64bit pointers :-(</p>
<p>I have an idea of other light hash structure (inspired by khash), but I do not bench it yet.</p>
<p>Any way, I think James's linked list for subclasses is most suitable for this task.<br>
Why change it to hash?</p> Ruby master - Feature #8426: Implement class hierarchy method cachinghttps://bugs.ruby-lang.org/issues/8426?journal_id=394582013-05-20T19:23:18ZAnonymous
<ul></ul><p>On Monday, 20 May 2013 at 7:39 PM, SASADA Koichi wrote:</p>
<blockquote>
<p>Is it huge performance bottleneck? I think branch on inline cache should be removed</p>
</blockquote>
<p>This helps a lot when Ruby programs are starting up because the full class hierarchy does not need to be traversed as often.</p>
<p>I'll rewrite the guard to be branch free and see if there is any performance improvement.</p>
<blockquote>
<p>I prefer `st_' related name. But not strong opinion.<br>
I disagree because they are unrelated data structures.</p>
</blockquote>
<blockquote>
<p>One more:</p>
<p>if (LIKELY(GET_METHOD_STATE_VERSION() == ci->vmstat &&<br>
RCLASS_EXT(klass)->seq == ci->seq &&<br>
klass == ci->klass)) {</p>
<p>should be:</p>
<p>if (LIKELY(GET_METHOD_STATE_VERSION() == ci->vmstat &&<br>
klass == ci->klass &&<br>
RCLASS_EXT(klass)->seq == ci->seq) {</p>
</blockquote>
<p>I don't think the order of checks matters, except for maybe performance reasons. I'll experiment with making this branch free instead.</p> Ruby master - Feature #8426: Implement class hierarchy method cachinghttps://bugs.ruby-lang.org/issues/8426?journal_id=394612013-05-21T01:53:17Znormalperson (Eric Wong)normalperson@yhbt.net
<ul></ul><p>Charlie Somerville <a href="mailto:charlie@charliesomerville.com" class="email">charlie@charliesomerville.com</a> wrote:</p>
<blockquote>
<blockquote>
<p>I prefer `st_' related name. But not strong opinion.<br>
I disagree because they are unrelated data structures.</p>
</blockquote>
</blockquote>
<p>In any case, I strongly prefer new sa_* functions (and more importantly<br>
data-structures) not be publically visible to C extensions. Exposing<br>
st_* was a mistake (IMHO) and makes it harder to maintain compatibility<br>
while making internal improvements.</p>
<p>Also, I think "sa_" prefix is confusing since sigaction already uses it.<br>
Maybe "sary_"?</p> Ruby master - Feature #8426: Implement class hierarchy method cachinghttps://bugs.ruby-lang.org/issues/8426?journal_id=414322013-08-30T23:38:18ZAnonymous
<ul></ul><p>ko1, have you had a chance to review <a href="https://github.com/ruby/ruby/pull/387" class="external">https://github.com/ruby/ruby/pull/387</a> ?</p>
<p>Thanks</p> Ruby master - Feature #8426: Implement class hierarchy method cachinghttps://bugs.ruby-lang.org/issues/8426?journal_id=414332013-08-31T00:14:03Znobu (Nobuyoshi Nakada)nobu@ruby-lang.org
<ul></ul><p>Why do you remove prototype declarations in ruby/encoding.h, but add old K&R style declarations instead?</p> Ruby master - Feature #8426: Implement class hierarchy method cachinghttps://bugs.ruby-lang.org/issues/8426?journal_id=414342013-08-31T00:38:00ZAnonymous
<ul></ul><p>nobu: I see you've already fixed the problem. I've removed the commit that changes ruby/encoding.h from the pull request.</p> Ruby master - Feature #8426: Implement class hierarchy method cachinghttps://bugs.ruby-lang.org/issues/8426?journal_id=416052013-09-04T14:25:13ZAnonymous
<ul><li><strong>Status</strong> changed from <i>Open</i> to <i>Closed</i></li><li><strong>% Done</strong> changed from <i>0</i> to <i>100</i></li></ul><p>This issue was solved with changeset r42822.<br>
Charlie, thank you for reporting this issue.<br>
Your contribution to Ruby is greatly appreciated.<br>
May Ruby be with you.</p>
<hr>
<ul>
<li>
<p>class.c, compile.c, eval.c, gc.h, insns.def, internal.h, method.h,<br>
variable.c, vm.c, vm_core.c, vm_insnhelper.c, vm_insnhelper.h,<br>
vm_method.c: Implement class hierarchy method cache invalidation.</p>
<p><a href="/issues/8426">[ruby-core:55053]</a> [Feature <a class="issue tracker-2 status-5 priority-4 priority-default closed" title="Feature: Implement class hierarchy method caching (Closed)" href="https://bugs.ruby-lang.org/issues/8426">#8426</a>] [GH-387]</p>
</li>
</ul> Ruby master - Feature #8426: Implement class hierarchy method cachinghttps://bugs.ruby-lang.org/issues/8426?journal_id=456962014-03-09T05:10:26Znormalperson (Eric Wong)normalperson@yhbt.net
<ul></ul><p>I noticed this was reverted in r43027 for being too slow.<br>
Is there a plan to improve and reintroduce it?</p>
<p>I may try adding caching in the main method table itself;<br>
especially if we end up using the container_of-style of method tables<br>
from Feature <a class="issue tracker-2 status-1 priority-4 priority-default" title="Feature: ordering of non-Hash items which use st_ internally (Open)" href="https://bugs.ruby-lang.org/issues/9614">#9614</a> to reduce indirection.</p> Ruby master - Feature #8426: Implement class hierarchy method cachinghttps://bugs.ruby-lang.org/issues/8426?journal_id=457012014-03-09T10:30:21Zfunny_falcon (Yura Sokolov)funny.falcon@gmail.com
<ul></ul><p>parallel/continuation of this issue is in <a href="https://bugs.ruby-lang.org/issues/9262" class="external">https://bugs.ruby-lang.org/issues/9262</a></p> Ruby master - Feature #8426: Implement class hierarchy method cachinghttps://bugs.ruby-lang.org/issues/8426?journal_id=457102014-03-10T06:58:41Znormalperson (Eric Wong)normalperson@yhbt.net
<ul></ul><p>Eric Wong <a href="mailto:normalperson@yhbt.net" class="email">normalperson@yhbt.net</a> wrote:</p>
<blockquote>
<p>I may try adding caching in the main method table itself;<br>
especially if we end up using the container_of-style of method tables</p>
</blockquote>
<p>Tried and unimpressive on bm_so_binary_trees so far:<br>
<a href="http://bogomips.org/ruby.git/patch?id=a5ea40b8f6550ceff58781d" class="external">http://bogomips.org/ruby.git/patch?id=a5ea40b8f6550ceff58781d</a></p>
<blockquote>
<p>from Feature <a class="issue tracker-2 status-1 priority-4 priority-default" title="Feature: ordering of non-Hash items which use st_ internally (Open)" href="https://bugs.ruby-lang.org/issues/9614">#9614</a> to reduce indirection.</p>
</blockquote>
<p>At least that saves memory...</p>