WIP Hackdays method dispatch

[methodmissing]

On digging a little deeper in a Intel VTune Amplifier profiler I noticed the memory bound line referencing argc on the rb_call_info struct.

Looking at the struct layout in master we have:

lourens@CarbonX1:~/src/ruby/trunk$ pahole -C rb_call_info ./miniruby
struct rb_call_info {
	ID                         mid;                  /*     0     8 */
	unsigned int               flag;                 /*     8     4 */
	int                        orig_argc;            /*    12     4 */

	/* size: 16, cachelines: 1, members: 3 */
	/* last cacheline: 16 bytes */
};
lourens@CarbonX1:~/src/ruby/trunk$ pahole -C rb_call_cache ./miniruby
struct rb_call_cache {
	rb_serial_t                method_state;         /*     0     8 */
	rb_serial_t                class_serial[3];      /*     8    24 */
	const struct rb_callable_method_entry_struct  * me; /*    32     8 */
	const struct rb_method_definition_struct  * def; /*    40     8 */
	VALUE                      (*call)(struct rb_execution_context_struct *, struct rb_control_frame_struct *, struct rb_calling_info *, struct rb_call_data *); /*    48     8 */
	union {
		unsigned int       index;                /*    56     4 */
		enum method_missing_reason method_missing_reason; /*    56     4 */
	} aux;                                           /*    56     4 */

	/* size: 64, cachelines: 1, members: 6 */
	/* padding: 4 */
};
lourens@CarbonX1:~/src/ruby/trunk$ pahole -C rb_call_data ./miniruby
struct rb_call_data {
	struct rb_call_cache cc;                         /*     0    64 */

	/* XXX last struct has 4 bytes of padding */

	/* --- cacheline 1 boundary (64 bytes) --- */
	struct rb_call_info ci;                          /*    64    16 */

	/* size: 80, cachelines: 2, members: 2 */
	/* paddings: 1, sum paddings: 4 */
	/* last cacheline: 16 bytes */
};

Conclusions:

• rb_call_data is a wrapper / container for both the inline method cache and the "static" rb_call_info struct (ideally should be qualified as const, but it changes state at runtime and hydrating from dumped bytecode is complicated otherwise)
• This looks like the classic hot / cold struct split, HOWEVER only the inline method call cache fits into a single cache line with the whole call info struct spilling over into the next.
• If we look at rb_call_cache layout, there's 4 bytes of padding up for grabs to cache additional concerns
• A skim through the VM source revealed that the orig_argc and flag members are referenced regularly at runtime, with a bit of chicken and egg going on with this hot / cold struct container split.

Approaches taken:

• Given the set of flags currently used being minimal, a 2 byte short int would suffice for a flag member mirroring the flag member on rb_call_info
• Given the maximum number of arguments ruby supports is 16 (update: for cfuncs only, max 65k using unsigned short int, see WIP Hackdays method dispatch #3 (comment)), a 2 byte short int would also work for mirroring orig_argc from the call info to the cache struct.
• The 4 byte padding is perfect for this.

Layout after:

lourens@CarbonX1:~/src/ruby/ruby$ pahole -C rb_call_info ./miniruby
struct rb_call_info {
	ID                         mid;                  /*     0     8 */
	short unsigned int         flag;                 /*     8     2 */
	short int                  orig_argc;            /*    10     2 */

	/* size: 16, cachelines: 1, members: 3 */
	/* padding: 4 */
	/* last cacheline: 16 bytes */
};
lourens@CarbonX1:~/src/ruby/ruby$ pahole -C rb_call_cache ./miniruby
struct rb_call_cache {
	rb_serial_t                method_state;         /*     0     8 */
	rb_serial_t                class_serial[3];      /*     8    24 */
	const struct rb_callable_method_entry_struct  * me; /*    32     8 */
	const struct rb_method_definition_struct  * def; /*    40     8 */
	VALUE                      (*call)(struct rb_execution_context_struct *, struct rb_control_frame_struct *, struct rb_calling_info *, struct rb_call_data *); /*    48     8 */
	short int                  orig_argc;            /*    56     2 */
	short int                  flag;                 /*    58     2 */
	union {
		unsigned int       index;                /*    60     4 */
		enum method_missing_reason method_missing_reason; /*    60     4 */
	} aux;                                           /*    60     4 */

	/* size: 64, cachelines: 1, members: 8 */
};
lourens@CarbonX1:~/src/ruby/ruby$ pahole -C rb_call_data ./miniruby
struct rb_call_data {
	struct rb_call_cache cc;                         /*     0    64 */
	/* --- cacheline 1 boundary (64 bytes) --- */
	struct rb_call_info ci;                          /*    64    16 */

	/* XXX last struct has 4 bytes of padding */

	/* size: 80, cachelines: 2, members: 2 */
	/* paddings: 1, sum paddings: 4 */
	/* last cacheline: 16 bytes */
};

Conclusions:

• I tried to shrink the call info struct's ints to short ints, but it doesn't make a size difference to the struct, cause well, padding.
• Therefore the containing call data struct also doesn't change in size (I'll revert this)
• We do however end up with a nice fully packed rb_call_cache struct and managed to sneak the flag and orig_argc in.

Direction of this optimization:

• Try to use a single cache line for most call and cache data specific operations
• Needs to also be backwards compatible with dumped bytecode (dumped as call info, hydrated as call data and the cache member needs to reflect the new flag and argc members too)
• Reduce locals referencing rb_call_info structs in some of the vm_ (hot, supports iseqs directly) functions
• Allow flag checks to be interchangeable with either rb_call_info or rb_cache_info as they both have the same member and not all helper methods pass call data OR call cache as arguments.
• This approach does not clobber the mjit implementation

I'll do some further testing tomorrow.

[methodmissing]

History of the structure splits:

Sun Sep 20 02:46:34 2015  Koichi Sasada  <ko1@atdot.net>
	* vm_core.h: split rb_call_info_t into several structs.
	  * rb_call_info (ci) has compiled fixed information.
	  * if ci->flag & VM_CALL_KWARG, then rb_call_info is
	    also rb_call_info_with_kwarg. This technique reduce one word
	    for major rb_call_info data.
	  * rb_calling_info has temporary data (argc, blockptr, recv).
	    for each method dispatch. This data is allocated only on
	    machine stack.
	  * rb_call_cache is for inline method cache.
	  Before this patch, only rb_call_info_t data is passed.
	  After this patch, above three structs are passed.
	  This patch improves:
	  * data locality (rb_call_info is now read-only data).
	  * reduce memory consumption (rb_call_info_with_kwarg,
	    rb_calling_info).
	* compile.c: use above data.
	* insns.def: ditto.
	* iseq.c: ditto.
	* vm_args.c: ditto.
	* vm_eval.c: ditto.
	* vm_insnhelper.c: ditto.
	* vm_insnhelper.h: ditto.
	* iseq.h: add iseq_compile_data::ci_index and
	  iseq_compile_data::ci_kw_index.
	* tool/instruction.rb: introduce TS_CALLCACHE operand type.

If it's possible to get ID mid into the cache one, then rb_call_data falls away and just 1 cache line for everything method info and cache related.

[methodmissing]

Current cache miss stats, I find the refinements one interesting @ 0 for a Rails process - rb_call_cache has an 8 byte def member just for that and if there's a workaround here, that can be repurposed for ID mid instead

Exiting
[RUBY_DEBUG_COUNTER]	4179 normal exit.
[RUBY_DEBUG_COUNTER]	mc_inline_hit                 	      54683229
[RUBY_DEBUG_COUNTER]	mc_inline_miss                	       6114388
[RUBY_DEBUG_COUNTER]	mc_global_hit                 	      14806735
[RUBY_DEBUG_COUNTER]	mc_global_miss                	       2062697
[RUBY_DEBUG_COUNTER]	mc_global_state_miss          	        387724
[RUBY_DEBUG_COUNTER]	mc_class_serial_miss          	       5726664
[RUBY_DEBUG_COUNTER]	mc_cme_complement             	       3259129
[RUBY_DEBUG_COUNTER]	mc_cme_complement_hit         	       3256326
[RUBY_DEBUG_COUNTER]	mc_search_super               	      35322200
[RUBY_DEBUG_COUNTER]	mc_miss_by_nome               	           348
[RUBY_DEBUG_COUNTER]	mc_miss_by_distinct           	       2341956
[RUBY_DEBUG_COUNTER]	mc_miss_by_refine             	             0
[RUBY_DEBUG_COUNTER]	mc_miss_by_visi               	        329615
[RUBY_DEBUG_COUNTER]	mc_miss_spurious              	       3442469

[methodmissing]

After discussing with @casperisfine , the 16 argc limit is for cfuncs only. I changed orig_argc to unsigned short int which is still 65535 args. Violating that in theory and practice is possible, but there'd be larger problems. To revisit.

Jean Boussier Today at 9:52 AM
Given the maximum number of arguments ruby supports is 16,
Really?

Lourens Naudé 28 minutes ago
yes, sec for confirmation of that

Lourens Naudé 28 minutes ago
this is why these discussions are so nice - fact checking 😄

Jean Boussier 27 minutes ago
def a(b, c, d, e, f, g, h, i, j, k, l, m, n, o, p, q, v, w, x, y, z)
p z
end
a(1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21)

Jean Boussier 27 minutes ago
Seem to work fine

Lourens Naudé 27 minutes ago
yeah, confirming - I last bumped into this a year ago

Jean Boussier 26 minutes ago
Ran fine on 2.3 trying a newer one

Lourens Naudé 26 minutes ago
But you know it, :rubocop: rules for sure catches this anywhere @ shop already, right?

Jean Boussier 25 minutes ago
No idea

Jean Boussier 25 minutes ago
I mean I see why a linter wouldn't want it

Jean Boussier 25 minutes ago
but not why a language would limit it

Jean Boussier 25 minutes ago
especially when we got lots of methods taking *splats

Lourens Naudé 22 minutes ago
trying to remember if it was for cfuncs only

Lourens Naudé 21 minutes ago
can let it be unsigned short ints, 65k args

Jean Boussier 21 minutes ago
👌

Lourens Naudé 21 minutes ago
I mean at that point there's a code hygiene problem already

Jean Boussier 21 minutes ago
sounds reasonable

Lourens Naudé 21 minutes ago
still circling back on this - gimme a few

Jean Boussier 20 minutes ago
640K ought to be enough for anyone.

Jean Boussier 20 minutes ago
More seriously, yes a method taking more than 16 arguments is silly, but such low limitation would be just as silly

Jean Boussier 19 minutes ago
50 or 64 probably nobody would complian as long as it doesn't impact splats

Lourens Naudé 3 minutes ago
ok, found it - https://github.com/ruby/ruby/blob/master/include/ruby/intern.h#L1072

include/ruby/intern.h:1072
#define RB_METHOD_DEFINITION_DECL(def,nonnull,decl,vars)
https://github.com/ruby/ruby|ruby/rubyruby/ruby | Added by GitHub

Lourens Naudé 3 minutes ago
references cfuncs

Jean Boussier 3 minutes ago
That makes more sense

Lourens Naudé 3 minutes ago
https://github.com/ruby/ruby/blob/b753929806d0e42cdfde3f1a8dcdbf678f937e44/include/ruby/ruby.h#L2775

include/ruby/ruby.h:2775
RB_METHOD_DEFINITION_DECL(rb_define_method, (2,3), (VALUE klass, const char *name), (klass, name))
https://github.com/ruby/ruby|ruby/rubyruby/ruby | Added by GitHub

Lourens Naudé 2 minutes ago
thx for poking @ this, I'll amend with a comment and link the original statement there

[methodmissing]

Managed to pack the method ID on the rb_call_cache struct as well (not used yet, see below) in 62c27ee

• It doesn't make much sense to carry around the method definition member JUST for the refinement check
• Opted instead for flagging that at compile time with a new VM_CALL_REFINED flag and have the inline method cache check for and reset @ runtime
• This frees up the 8 bytes needed to pack ID mid (method id) onto the cache struct as well and subsequently all of rb_call_info has been packed into rb_call_cache, effectively constraining memory access patterns of rb_call_data during dispatch to just the first cache line

Tackling integrating this method ID with dispatch now.

lourens@CarbonX1:~/src/ruby/ruby$ pahole -C rb_call_cache ./miniruby
struct rb_call_cache {
	rb_serial_t                method_state;         /*     0     8 */
	rb_serial_t                class_serial[3];      /*     8    24 */
	const struct rb_callable_method_entry_struct  * me; /*    32     8 */
	ID                         mid;                  /*    40     8 */ <<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<
	VALUE                      (*call)(struct rb_execution_context_struct *, struct rb_control_frame_struct *, struct rb_calling_info *, struct rb_call_data *); /*    48     8 */
	short unsigned int         orig_argc;            /*    56     2 */
	short unsigned int         flag;                 /*    58     2 */
	union {
		unsigned int       index;                /*    60     4 */
		enum method_missing_reason method_missing_reason; /*    60     4 */
	} aux;                                           /*    60     4 */

	/* size: 64, cachelines: 1, members: 8 */
};

[methodmissing]

Tried an inverse approach with flag VM_CALL_UNREFINED in 73c819f instead and let method dispatch prefer method ID from the call cache instead of call info. Also managed to remove a few local declarations of rb_call_info * types

[methodmissing]

Local test failures fixed in 3edf237 , but need to pay attention to the JIT ones still:

Fiber count: 10000 (skipping)
Thread count: 9839 (can't create Thread: Resource temporarily unavailable)
PASS all 1409 tests
exec ./miniruby -I./lib -I. -I.ext/common  ./tool/runruby.rb --extout=.ext  -- --disable-gems "./bootstraptest/runner.rb" --ruby="ruby --disable-gems"   ./KNOWNBUGS.rb
2019-12-13 11:54:17 +0000
Driver is ruby 2.7.0dev (2019-12-12T23:06:24Z hackdays-method-di.. 73c819f958) [x86_64-linux]
Target is ruby 2.7.0dev (2019-12-12T23:06:24Z hackdays-method-di.. 73c819f958) [x86_64-linux]

KNOWNBUGS.rb  PASS 0
No tests, no problem

test succeeded
Run options: "--ruby=./miniruby -I./lib -I. -I.ext/common  ./tool/runruby.rb --extout=.ext  -- --disable-gems"

# Running tests:

Finished tests in 2.551074s, 89.7661 tests/s, 195.9959 assertions/s.                                            
229 tests, 500 assertions, 0 failures, 0 errors, 0 skips

ruby -v: ruby 2.7.0dev (2019-12-12T23:06:24Z hackdays-method-di.. 73c819f958) [x86_64-linux]
Run options: "--ruby=./miniruby -I./lib -I. -I.ext/common  ./tool/runruby.rb --extout=.ext  -- --disable-gems" --excludes-dir=./test/excludes --name=!/memory_leak/

# Running tests:

Leaked thread: DRbTests::TestDRbSSLCore#test_10_yield: #<Thread:0x000055b8e37dfe00@/home/lourens/src/ruby/ruby/lib/drb/drb.rb:1258 sleep_forever>
Finished thread: DRbTests::TestDRbSSLCore#test_10_yield_undumped: #<Thread:0x000055b8e37dfe00@/home/lourens/src/ruby/ruby/lib/drb/drb.rb:1258 dead>
Leaked file descriptor: DRbTests::TestDRbTCP#test_immediate_close: 10 : #<IO:fd 10>     
Finished tests in 444.155192s, 46.9701 tests/s, 6127.8536 assertions/s.                                                              
20862 tests, 2721718 assertions, 0 failures, 0 errors, 65 skips

ruby -v: ruby 2.7.0dev (2019-12-12T23:06:24Z hackdays-method-di.. 73c819f958) [x86_64-linux]
$ /home/lourens/src/ruby/ruby/miniruby -I/home/lourens/src/ruby/ruby/lib /home/lourens/src/ruby/ruby/tool/runruby.rb --archdir=/home/lourens/src/ruby/ruby --extout=.ext -- /home/lourens/src/ruby/ruby/spec/mspec/bin/mspec-run -B ./spec/default.mspec
ruby 2.7.0dev (2019-12-12T23:06:24Z hackdays-method-di.. 73c819f958) [x86_64-linux]
[| | ==================100%================== | 00:00:00]      0F      0E 

Finished in 60.285608 seconds

3745 files, 30411 examples, 146687 expectations, 0 failures, 0 errors, 0 tagged
check succeeded

[methodmissing]

First larger app / Rails test:

lourens@CarbonX1:~/src/redmine$ ruby -v
ruby 2.7.0dev (2019-12-11T22:43:38Z hackdays-method-di.. a6e27ce15b) [x86_64-linux]

Booted redmine fine:

[2019-12-13 12:14:35] INFO  WEBrick 1.6.0
[2019-12-13 12:14:35] INFO  ruby 2.7.0 (2019-12-09) [x86_64-linux]
[2019-12-13 12:14:35] INFO  WEBrick::HTTPServer#start: pid=29822 port=3000

Processed 1750 requests without any issues:

lourens@CarbonX1:~/src/redmine$ wrk -c 2 -d 60 http://localhost:3000
Running 1m test @ http://localhost:3000
  2 threads and 2 connections
  Thread Stats   Avg      Stdev     Max   +/- Stdev
    Latency    82.47ms  118.03ms   1.41s    97.99%
    Req/Sec    14.90      5.03    20.00     50.90%
  1750 requests in 1.00m, 9.32MB read
Requests/sec:     29.12
Transfer/sec:    158.83KB

CI failing on a windows package repo error though:

Err:41 https://packages.microsoft.com/ubuntu/16.04/prod xenial/main amd64 Packages
  Hash Sum mismatch
Get:42 https://packages.cloud.google.com/apt kubernetes-xenial/main amd64 Packages [32.2 kB]
Get:44 http://ppa.launchpad.net/git-core/ppa/ubuntu xenial/main amd64 Packages [3,340 B]
Get:45 http://ppa.launchpad.net/hvr/ghc/ubuntu xenial/main amd64 Packages [16.0 kB]
Get:46 https://packagecloud.io/github/git-lfs/ubuntu xenial/main amd64 Packages [7,521 B]
Get:47 http://ppa.launchpad.net/ondrej/php/ubuntu xenial/main amd64 Packages [58.1 kB]
Fetched 5,234 kB in 2s (1,860 kB/s)
Reading package lists...
E: Failed to fetch https://packages.microsoft.com/ubuntu/16.04/prod/dists/xenial/main/binary-amd64/Packages.bz2  Hash Sum mismatch

[maximecb]

Should we close this PR?