New method cache mechanism for Guild and more #2888

ko1 · 2020-02-07T06:52:59Z

https://bugs.ruby-lang.org/issues/16614 for details.

k0kubun · 2020-02-07T09:21:19Z

test/-ext-/tracepoint/test_tracepoint.rb

  end

  def test_tracks_objspace_count
+    return


k0kubun · 2020-02-07T09:23:46Z

vm_insnhelper.c

 MJIT_FUNC_EXPORTED void
 vm_check_canary(const rb_execution_context_t *ec, VALUE *sp)
 {
+    return;


Please don't!

k0kubun · 2020-02-07T09:31:10Z

debug_counter.h

 #define RB_DEBUG_COUNTER_INC_UNLESS(type, cond) (!rb_debug_counter_add(RB_DEBUG_COUNTER_##type, 1, !(cond)))
 #define RB_DEBUG_COUNTER_INC_IF(type, cond)       rb_debug_counter_add(RB_DEBUG_COUNTER_##type, 1, (cond))
+#define RB_DEBUG_COUNTER_ADD(type, num)           rb_debug_counter_add(RB_DEBUG_COUNTER_##type, (num), 1)
+#define RB_DEBUG_COUNTER_SETMAX(type, num)        rb_debug_counter_max(RB_DEBUG_COUNTER_##type, (unsigned int)(num))


Are we using this somewhere?

Now they are not used, but will be used to add not incremental only counters.
For example, SETMAX is used to find maximum length of ccs.

k0kubun · 2020-02-07T09:33:33Z

mjit_worker.c

    CRITICAL_SECTION_FINISH(3, "in mjit_copy_cache_from_main_thread");

-    job->cc_entries = cc_entries;
+    job->cc_entries = (void *)cc_entries; // TODO


Is this still inevitable?

2 points.
(1) we need to copy cc_entries for GC. I'll write it in comment. f480167
(2) type mimatch because of const, so it is needed to solve.

class.c

shyouhei · 2020-02-12T02:55:16Z

class.c

-	    if (BUILTIN_TYPE(m) == T_ICLASS) m = RBASIC(m)->klass;
-	    rb_module_add_to_subclasses_list(m, iclass);
+            if (BUILTIN_TYPE(m) == T_ICLASS) m = RBASIC(m)->klass;
+            rb_module_add_to_subclasses_list(m, iclass);


Is it possible to delete this kind of noisy indent changes before requesting review? Very difficult to read what is important and what isn't.

sorry my editor changed it automatically and i missed it. I don't have an idea how to remove this kind of indentation change. any commit option?

FYI: adding ?w=1 to the end of URL like https://github.com/ruby/ruby/pull/2888/files?w=1 will hide such diffs.

I wrote a script that reverts whitespace only changes and sent you a pull request using it.

shyouhei · 2020-02-12T02:58:29Z

compile.c

+        struct rb_call_data *cd = &iseq->body->call_data[i];
+        const struct rb_callinfo *ci = cd->ci;
+        const struct rb_callcache *cc = cd->cc;
+        if (cc != NULL && cc != vm_cc_empty()) {


Can I ask you why you have to check vm_cc_empty() again here? Is it because the check at TS_CALLDATA branch above miss something?

At first this code is for verify data structure and disabled at the normal case.
this check is introduced at debugging time and maybe not important. I think we can remove it, but double check can help in future debug (first check and second check represents what call_data should be in different form).

compile.c

vm_callinfo.h

shyouhei · 2020-02-12T04:02:06Z

vm_insnhelper.c

-    for (; i > 0; i--) {
-        cc->class_serial[i] = cc->class_serial[i - 1];
+    if (cd_owner) {
+        RB_OBJ_WRITE(cd_owner, &cd->cc, cc);


Multiple guilds can write this field at once, right? I guess that is what you call the "atomicity", and you think that works. Am I correct?

a guild can consistent CC so there is no problem.

shyouhei · 2020-02-12T04:08:00Z

vm_method.c

+void
+rb_clear_method_cache_all(void)
+{
+    rb_objspace_each_objects(invalidate_all_cc, NULL);


Is it faster to use rb_objspace_each_objects instead of rb_class_foreach_subclass here? I guess this implementation is slower when there are lots of objetcs. Not tested though.

We shouldn't use subclass data structure because we can remove subclasses data structure in future so I use this implementation.

Now it is inefficient implementation, but we can remove it. This function is only used by using and using can employ smarter invalidation I guess. Also using is rarely used method.

vm.c

Now, rb_call_info contains how to call the method with tuple of (mid, orig_argc, flags, kwarg). Most of cases, kwarg == NULL and mid+argc+flags only requires 64bits. So this patch packed rb_call_info to VALUE (1 word) on such cases. If we can not represent it in VALUE, then use imemo_callinfo which contains conventional callinfo (rb_callinfo, renamed from rb_call_info). iseq->body->ci_kw_size is removed because all of callinfo is VALUE size (packed ci or a pointer to imemo_callinfo). To access ci information, we need to use these functions: vm_ci_mid(ci), _flag(ci), _argc(ci), _kwarg(ci). struct rb_call_info_kw_arg is renamed to rb_callinfo_kwarg. rb_funcallv_with_cc() and rb_method_basic_definition_p_with_cc() is temporary removed because cd->ci should be marked.

This patch contains several ideas: (1) Disposable inline method cache (IMC) for race-free inline method cache * Making call-cache (CC) as a RVALUE (GC target object) and allocate new CC on cache miss. * This technique allows race-free access from parallel processing elements like RCU. (2) Introduce per-Class method cache (pCMC) * Instead of fixed-size global method cache (GMC), pCMC allows flexible cache size. * Caching CCs reduces CC allocation and allow sharing CC's fast-path between same call-info (CI) call-sites. (3) Invalidate an inline method cache by invalidating corresponding method entries (MEs) * Instead of using class serials, we set "invalidated" flag for method entry itself to represent cache invalidation. * Compare with using class serials, the impact of method modification (add/overwrite/delete) is small. * Updating class serials invalidate all method caches of the class and sub-classes. * Proposed approach only invalidate the method cache of only one ME. See [Feature #16614] for more details.

Since #2888 this macro is no longer used in any place.

Thanks to Ractor (#2888 and #3662), inline caches now support parallel access.

Thanks to Ractor (ruby#2888 and ruby#3662), inline caches support parallel access now.

Thanks to Ractor (#2888 and #3662), inline caches support parallel access now.

It doesn't do anything since ruby 3.0.0.preview1. It was removed in ruby/ruby#2888

ko1 force-pushed the value_cc branch from 91a300f to ba8c258 Compare February 7, 2020 09:04

ko1 marked this pull request as ready for review February 7, 2020 09:05

ko1 requested review from k0kubun, nobu and shyouhei February 7, 2020 09:05

k0kubun reviewed Feb 7, 2020

View reviewed changes

shyouhei reviewed Feb 12, 2020

View reviewed changes

ko1 commented Feb 12, 2020

View reviewed changes

vm.c Outdated Show resolved Hide resolved

ko1 force-pushed the value_cc branch 3 times, most recently from 4a98ce3 to b98a947 Compare February 14, 2020 07:30

ko1 force-pushed the value_cc branch from a41dc16 to e9787dd Compare February 21, 2020 18:08

ko1 force-pushed the value_cc branch from e9787dd to 0fb00f5 Compare February 21, 2020 18:34

ko1 merged commit b9007b6 into ruby:master Feb 22, 2020

ko1 deleted the value_cc branch February 22, 2020 00:59

yahonda mentioned this pull request Mar 1, 2020

Action Text and Action View unit tests are failing with ruby 2.8.0dev rails/rails#38613

Closed

matzbot pushed a commit that referenced this pull request Apr 13, 2020

delete CACHELINE

5dc6080

Since #2888 this macro is no longer used in any place.

k0kubun added a commit that referenced this pull request Nov 21, 2020

Eliminate IVC sync between JIT and Ruby threads

342c093

Thanks to Ractor (#2888 and #3662), inline caches now support parallel access.

k0kubun mentioned this pull request Nov 21, 2020

Eliminate IVC sync between JIT and Ruby threads #3798

Closed

k0kubun added a commit to k0kubun/ruby that referenced this pull request Nov 21, 2020

Eliminate IVC sync between JIT and Ruby threads

1300355

Thanks to Ractor (ruby#2888 and ruby#3662), inline caches support parallel access now.

k0kubun mentioned this pull request Nov 21, 2020

Eliminate IVC sync between JIT and Ruby threads #3799

Merged

k0kubun added a commit that referenced this pull request Nov 21, 2020

Eliminate IVC sync between JIT and Ruby threads (#3799)

0960f56

Thanks to Ractor (#2888 and #3662), inline caches support parallel access now.

cvx added a commit to discourse/discourse that referenced this pull request Apr 26, 2023

DEV: Remove RUBY_GLOBAL_METHOD_CACHE_SIZE

ef4234f

It doesn't do anything since ruby 3.0.0.preview1. It was removed in ruby/ruby#2888

cvx mentioned this pull request Apr 26, 2023

DEV: Remove RUBY_GLOBAL_METHOD_CACHE_SIZE discourse/discourse#21249

Merged

cvx added a commit to discourse/discourse_docker that referenced this pull request Apr 26, 2023

DEV: Remove RUBY_GLOBAL_METHOD_CACHE_SIZE

ad98103

It doesn't do anything since ruby 3.0.0.preview1. It was removed in ruby/ruby#2888

cvx mentioned this pull request Apr 26, 2023

DEV: Remove RUBY_GLOBAL_METHOD_CACHE_SIZE discourse/discourse_docker#711

Merged

cvx added a commit to discourse/discourse that referenced this pull request Apr 26, 2023

DEV: Remove RUBY_GLOBAL_METHOD_CACHE_SIZE (#21249)

00630e4

It doesn't do anything since ruby 3.0.0.preview1. It was removed in ruby/ruby#2888

ChadBoschert pushed a commit to apt-crowd/discourse that referenced this pull request Apr 26, 2023

DEV: Remove RUBY_GLOBAL_METHOD_CACHE_SIZE (discourse#21249)

b05f4c2

It doesn't do anything since ruby 3.0.0.preview1. It was removed in ruby/ruby#2888

cvx added a commit to discourse/discourse_docker that referenced this pull request May 9, 2023

DEV: Remove RUBY_GLOBAL_METHOD_CACHE_SIZE (#711)

7eaf87a

It doesn't do anything since ruby 3.0.0.preview1. It was removed in ruby/ruby#2888

New method cache mechanism for Guild and more #2888

New method cache mechanism for Guild and more #2888

Uh oh!

Conversation

ko1 commented Feb 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ko1 Feb 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ko1 commented Feb 7, 2020 •

edited

Loading

ko1 Feb 12, 2020 •

edited

Loading