[PATCH v2] Allow Raindrops objects to be backed by a file

[PATCH v2] Allow Raindrops objects to be backed by a file

[KJ Tsanaktsidis]

Thanks for your feedback on my previous patch Eric, I think backing
Raindrops by an arbitrary file (which can actually be on tmpfs) is much
neater than harcdoding some stuff in there for memfd.

Looking forward to hearing your thoughts on this one!

---------------------------------------

Currently, all memory used by Raindrops is mapped as MAP_ANONYMOUS. This
means that although Raindrops counters can be shared between processes
that have forked from each other, it is not possible to share the
counter values with another, unrelated process.

This patch adds support for backing the Raindrops mapping with a file
descriptor obtained from an IO object. The #initialize API has been
enhanced with two new keyword options:

  Raindrops.new(size, io: nil, zero: false)

If an instance of IO is provided, then the underlying file descriptor
for that IO will be used to back the memory mapping Raindrops creates.
An unrelated process can then open the same file, and read the counters;
either by mmap'ing the file itself (or using Raindrops to do so), or by
making ordinary seek()/read() calls if performance is not a concern.

Note theat the provided IO object _must_ implement #truncate; this is
used to set the size of the file to be right-sized for the memory
mapping that is created.

If the zero argument is passed as true, then the mapping will be zero'd
by Raindrops as part of its initialization. If it's false, then the
Raindrops counters existing in the file will be preserved. This allows
counter values to be persisted (although note that Raindrops makes no
attempt to msync the values, so they are not durable to e.g. system
crashes).

On Linux, counter values can easily be shared between processes
in-memory only without touching the disk by passing in a File on a
tmpfs as the io object.
---
 ext/raindrops/raindrops.c | 63 +++++++++++++++++++++++++++++----------
 lib/raindrops.rb          | 24 +++++++++++++++
 test/test_raindrops.rb    | 42 ++++++++++++++++++++++++++
 3 files changed, 114 insertions(+), 15 deletions(-)

diff --git a/ext/raindrops/raindrops.c b/ext/raindrops/raindrops.c
index 837084c..8cd9ed2 100644
--- a/ext/raindrops/raindrops.c
+++ b/ext/raindrops/raindrops.c
@@ -4,6 +4,7 @@
 #include <assert.h>
 #include <errno.h>
 #include <stddef.h>
+#include <string.h>
 #include "raindrops_atomic.h"
 
 #ifndef SIZET2NUM
@@ -34,9 +35,17 @@ struct raindrops {
 	size_t size;
 	size_t capa;
 	pid_t pid;
+	VALUE io;
 	struct raindrop *drops;
 };
 
+/* called by GC */
+static void rd_mark(void *ptr)
+{
+	struct raindrops *r = ptr;
+	rb_gc_mark(r->io);
+}
+
 /* called by GC */
 static void rd_free(void *ptr)
 {
@@ -60,7 +69,7 @@ static size_t rd_memsize(const void *ptr)
 
 static const rb_data_type_t rd_type = {
 	"raindrops",
-	{ NULL, rd_free, rd_memsize, /* reserved */ },
+	{ rd_mark, rd_free, rd_memsize, /* reserved */ },
 	/* parent, data, [ flags ] */
 };
 
@@ -87,16 +96,10 @@ static struct raindrops *get(VALUE self)
 }
 
 /*
- * call-seq:
- *	Raindrops.new(size)	-> raindrops object
- *
- * Initializes a Raindrops object to hold +size+ counters.  +size+ is
- * only a hint and the actual number of counters the object has is
- * dependent on the CPU model, number of cores, and page size of
- * the machine.  The actual size of the object will always be equal
- * or greater than the specified +size+.
+ * This is the _acutal_ implementation of #initialize - the Ruby wrapper
+ * handles keyword-argument handling then calls this method.
  */
-static VALUE init(VALUE self, VALUE size)
+static VALUE init_cimpl(VALUE self, VALUE size, VALUE io, VALUE zero)
 {
 	struct raindrops *r = DATA_PTR(self);
 	int tries = 1;
@@ -113,9 +116,18 @@ static VALUE init(VALUE self, VALUE size)
 	r->capa = tmp / raindrop_size;
 	assert(PAGE_ALIGN(raindrop_size * r->capa) == tmp && "not aligned");
 
+	r->io = io;
+
 retry:
-	r->drops = mmap(NULL, tmp,
-	                PROT_READ|PROT_WRITE, MAP_ANON|MAP_SHARED, -1, 0);
+	if (RTEST(r->io)) {
+		rb_funcall(r->io, rb_intern("truncate"), 1, SIZET2NUM(tmp));
+		int fd = NUM2INT(rb_funcall(r->io, rb_intern("fileno"), 0));
+		r->drops = mmap(NULL, tmp,
+				PROT_READ|PROT_WRITE, MAP_SHARED, fd, 0);
+	} else {
+		r->drops = mmap(NULL, tmp,
+				PROT_READ|PROT_WRITE, MAP_ANON|MAP_SHARED, -1, 0);
+	}
 	if (r->drops == MAP_FAILED) {
 		int err = errno;
 
@@ -127,6 +139,10 @@ retry:
 	}
 	r->pid = getpid();
 
+	if (RTEST(zero)) {
+		memset(r->drops, 0, tmp);
+	}
+
 	return self;
 }
 
@@ -217,14 +233,16 @@ static VALUE capa(VALUE self)
  * call-seq:
  *	rd.dup		-> rd_copy
  *
- * Duplicates and snapshots the current state of a Raindrops object.
+ * Duplicates and snapshots the current state of a Raindrops object. Even
+ * if the given Raindrops object is backed by a file, the copy will be backed
+ * by independent, anonymously mapped memory.
  */
 static VALUE init_copy(VALUE dest, VALUE source)
 {
 	struct raindrops *dst = DATA_PTR(dest);
 	struct raindrops *src = get(source);
 
-	init(dest, SIZET2NUM(src->size));
+	init_cimpl(dest, SIZET2NUM(src->size), Qnil, Qfalse);
 	memcpy(dst->drops, src->drops, raindrop_size * src->size);
 
 	return dest;
@@ -375,6 +393,20 @@ static VALUE evaporate_bang(VALUE self)
 	return Qnil;
 }
 
+/*
+ * call-seq:
+ * 	to_io	-> IO
+ *
+ * Returns the IO object backing the memory for this raindrop, if
+ * one was specified when constructing this Raindrop. If this
+ * Raindrop is backed by anonymous memory, this method returns nil.
+ */
+static VALUE to_io(VALUE self)
+{
+	struct raindrops *r = get(self);
+	return r->io;
+}
+
 void Init_raindrops_ext(void)
 {
 	VALUE cRaindrops = rb_define_class("Raindrops", rb_cObject);
@@ -433,7 +465,7 @@ void Init_raindrops_ext(void)
 
 	rb_define_alloc_func(cRaindrops, alloc);
 
-	rb_define_method(cRaindrops, "initialize", init, 1);
+	rb_define_private_method(cRaindrops, "initialize_cimpl", init_cimpl, 3);
 	rb_define_method(cRaindrops, "incr", incr, -1);
 	rb_define_method(cRaindrops, "decr", decr, -1);
 	rb_define_method(cRaindrops, "to_ary", to_ary, 0);
@@ -444,6 +476,7 @@ void Init_raindrops_ext(void)
 	rb_define_method(cRaindrops, "capa", capa, 0);
 	rb_define_method(cRaindrops, "initialize_copy", init_copy, 1);
 	rb_define_method(cRaindrops, "evaporate!", evaporate_bang, 0);
+	rb_define_method(cRaindrops, "to_io", to_io, 0);
 
 #ifdef __linux__
 	Init_raindrops_linux_inet_diag();
diff --git a/lib/raindrops.rb b/lib/raindrops.rb
index ba273eb..dc61952 100644
--- a/lib/raindrops.rb
+++ b/lib/raindrops.rb
@@ -36,6 +36,30 @@ class ListenStats < Struct.new(:active, :queued)
     def total
       active + queued
     end
+  end unless defined? ListenStats
+
+  # call-seq:
+  #	Raindrops.new(size, io: nil)	-> raindrops object
+  #
+  # Initializes a Raindrops object to hold +size+ counters.  +size+ is
+  # only a hint and the actual number of counters the object has is
+  # dependent on the CPU model, number of cores, and page size of
+  # the machine.  The actual size of the object will always be equal
+  # or greater than the specified +size+.
+  # If +io+ is provided, then the Raindrops memory will be backed by
+  # the specified file; otherwise, it will allocate anonymous memory.
+  # The IO object must respond to +truncate+, as this is used to set
+  # the size of the file.
+  # If +zero+ is provided, then the memory region is zeroed prior to
+  # returning. This is only meaningful if +io+ is also provided; in
+  # that case it controls whether any existing counter values in +io+
+  # are retained (false) or whether it is entirely zeroed (true).
+  def initialize(size, io: nil, zero: false)
+    # This ruby wrapper exists to handle the keyword-argument handling,
+    # which is otherwise kind of awkward in C. We delegate the keyword
+    # arguments to the _actual_ initialize implementation as positional
+    # args.
+    initialize_cimpl(size, io, zero)
   end
 
   autoload :Linux, 'raindrops/linux'
diff --git a/test/test_raindrops.rb b/test/test_raindrops.rb
index 0749694..6351c66 100644
--- a/test/test_raindrops.rb
+++ b/test/test_raindrops.rb
@@ -1,6 +1,7 @@
 # -*- encoding: binary -*-
 require 'test/unit'
 require 'raindrops'
+require 'tempfile'
 
 class TestRaindrops < Test::Unit::TestCase
 
@@ -162,4 +163,45 @@ def test_evaporate_with_fork
     assert status.success?
     assert_equal [ 1, 2 ], tmp.to_ary
   end
+
+  def test_io_backed
+    file = Tempfile.new('test_io_backed')
+    rd = Raindrops.new(4, io: file, zero: true)
+    rd[0] = 123
+    rd[1] = 456
+
+    assert_equal 123, rd[0]
+    assert_equal 456, rd[1]
+
+    rd.evaporate!
+
+    file.rewind
+    data = file.read
+    assert_equal 123, data.unpack('L!')[0]
+    assert_equal 456, data[Raindrops::SIZE..data.size].unpack('L!')[0]
+  end
+
+  def test_io_backed_reuse
+    file = Tempfile.new('test_io_backed')
+    rd = Raindrops.new(4, io: file, zero: true)
+    rd[0] = 123
+    rd[1] = 456
+    rd.evaporate!
+
+    rd = Raindrops.new(4, io: file, zero: false)
+    assert_equal 123, rd[0]
+    assert_equal 456, rd[1]
+  end
+
+  def test_iobacked_noreuse
+    file = Tempfile.new('test_io_backed')
+    rd = Raindrops.new(4, io: file, zero: true)
+    rd[0] = 123
+    rd[1] = 456
+    rd.evaporate!
+
+    rd = Raindrops.new(4, io: file, zero: true)
+    assert_equal 0, rd[0]
+    assert_equal 0, rd[1]
+  end
 end
-- 
2.33.1

Re: [PATCH v2] Allow Raindrops objects to be backed by a file

[Eric Wong]

KJ Tsanaktsidis <ktsanaktsidis@zendesk.com> wrote:
> Thanks for your feedback on my previous patch Eric, I think backing
> Raindrops by an arbitrary file (which can actually be on tmpfs) is much
> neater than harcdoding some stuff in there for memfd.
> 
> Looking forward to hearing your thoughts on this one!

Thanks.  A few minor things, more inline, but it looks good
and I'm inclined to squash the changes in the --range-diff
(below).

> ---------------------------------------
> 
> Currently, all memory used by Raindrops is mapped as MAP_ANONYMOUS. This

<snip>

Note that applies to other projects, too:  instead of a solid
"-------" line, using a "----8<----" line as below
(without indentation) for --scissors:

	<prose not intended for a git commit message>

	----8<----
	Subject: [PATCH] Allow Raindrops objects to be backed by a file

	<rest of the commit message + patch...>

The above allows maintainers to use `git am --scissors' to apply
a patch while ignoring any text above the "----8<----" line.

<snip>

> Note theat the provided IO object _must_ implement #truncate; this is

s/theat/that/  (I can squash this in on my end)

<snip>

> On Linux, counter values can easily be shared between processes
> in-memory only without touching the disk by passing in a File on a
> tmpfs as the io object.

AFAIK this applies to any OS with mmap support, and raindrops
already requires mmap.  I'll remove the "On Linux," part before
pushing.

<snip>

> - *	Raindrops.new(size)	-> raindrops object
> - *
> - * Initializes a Raindrops object to hold +size+ counters.  +size+ is
> - * only a hint and the actual number of counters the object has is
> - * dependent on the CPU model, number of cores, and page size of
> - * the machine.  The actual size of the object will always be equal
> - * or greater than the specified +size+.
> + * This is the _acutal_ implementation of #initialize - the Ruby wrapper

s/acutal/actual/

> + * handles keyword-argument handling then calls this method.
>   */
> -static VALUE init(VALUE self, VALUE size)
> +static VALUE init_cimpl(VALUE self, VALUE size, VALUE io, VALUE zero)
>  {
>  	struct raindrops *r = DATA_PTR(self);
>  	int tries = 1;
> @@ -113,9 +116,18 @@ static VALUE init(VALUE self, VALUE size)
>  	r->capa = tmp / raindrop_size;
>  	assert(PAGE_ALIGN(raindrop_size * r->capa) == tmp && "not aligned");
>  
> +	r->io = io;
> +
>  retry:
> -	r->drops = mmap(NULL, tmp,
> -	                PROT_READ|PROT_WRITE, MAP_ANON|MAP_SHARED, -1, 0);
> +	if (RTEST(r->io)) {
> +		rb_funcall(r->io, rb_intern("truncate"), 1, SIZET2NUM(tmp));
> +		int fd = NUM2INT(rb_funcall(r->io, rb_intern("fileno"), 0));

I'll swap the lines so "int fd = ..." is first in the block
to avoid declaration-after-statement problems with older compilers.

> +		r->drops = mmap(NULL, tmp,
> +				PROT_READ|PROT_WRITE, MAP_SHARED, fd, 0);
> +	} else {
> +		r->drops = mmap(NULL, tmp,
> +				PROT_READ|PROT_WRITE, MAP_ANON|MAP_SHARED, -1, 0);

And I'll wrap this at 80-columns wrap (I need giant fonts).

> @@ -127,6 +139,10 @@ retry:
>  	}
>  	r->pid = getpid();
>  
> +	if (RTEST(zero)) {
> +		memset(r->drops, 0, tmp);
> +	}

No need for {} to waste vertical space.  Again, I need giant
fonts :x   Omitting unnecessary {} also matches Linux kernel
and git.git coding style.

> - * Duplicates and snapshots the current state of a Raindrops object.
> + * Duplicates and snapshots the current state of a Raindrops object. Even
> + * if the given Raindrops object is backed by a file, the copy will be backed
> + * by independent, anonymously mapped memory.
>   */
>  static VALUE init_copy(VALUE dest, VALUE source)
>  {
>  	struct raindrops *dst = DATA_PTR(dest);
>  	struct raindrops *src = get(source);
>  
> -	init(dest, SIZET2NUM(src->size));
> +	init_cimpl(dest, SIZET2NUM(src->size), Qnil, Qfalse);
>  	memcpy(dst->drops, src->drops, raindrop_size * src->size);

Switching from file-backed to anonymous seems strange to me,
here, but I'm not sure of a better solution that's compatible
with Object#dup ...  I also don't know if anybody uses
Raindrops#dup, either.  So if you're satisfied with this
behavior, that's good enough for me :>

> + * 	to_io	-> IO
> + *
> + * Returns the IO object backing the memory for this raindrop, if
> + * one was specified when constructing this Raindrop. If this
> + * Raindrop is backed by anonymous memory, this method returns nil.
> + */
> +static VALUE to_io(VALUE self)
> +{
> +	struct raindrops *r = get(self);
> +	return r->io;
> +}

OK.  There may be a need to call "to_io" on r->io itself
(because Tempfile delegates, but isn't actually an IO object
itself).  I'm not sure, as examples found in the Ruby stdlib
seem to contradict each other:

* Zlib::GzipFile returns the ->io as-is:
  https://public-inbox.org/ruby-core/be5f148bcd9f/s/?b=ext/zlib/zlib.c#n3233

* csv calls #to_io:
  https://public-inbox.org/ruby-core/42e99435cbf2/s/?b=lib/csv.rb#n2102

I'm not sure which is correct, here, actually :x  I'm inclined to
just leave it as-is, for now, following zlib's example.  Any
future changes here can be done separately.

Everything else looked good.  Below is the range-diff of the
minor things I'll squash in before pushing (probably in a few
days).  No need to resend.

Also, was there anything else you had planned? (e.g. for memfd)
Otherwise, I'm thinking of tagging and releasing a new version
in a week or so (at least before Ruby 3.1 on Dec 25)

Thanks again.

Range-diff:
1:  79be82a ! 1:  18ee38a Allow Raindrops objects to be backed by a file
    @@ Commit message
         either by mmap'ing the file itself (or using Raindrops to do so), or by
         making ordinary seek()/read() calls if performance is not a concern.
     
    -    Note theat the provided IO object _must_ implement #truncate; this is
    +    Note that the provided IO object _must_ implement #truncate; this is
         used to set the size of the file to be right-sized for the memory
         mapping that is created.
     
    @@ Commit message
         attempt to msync the values, so they are not durable to e.g. system
         crashes).
     
    -    On Linux, counter values can easily be shared between processes
    +    Counter values can easily be shared between processes
         in-memory only without touching the disk by passing in a File on a
         tmpfs as the io object.
     
    @@ ext/raindrops/raindrops.c: static struct raindrops *get(VALUE self)
     - * dependent on the CPU model, number of cores, and page size of
     - * the machine.  The actual size of the object will always be equal
     - * or greater than the specified +size+.
    -+ * This is the _acutal_ implementation of #initialize - the Ruby wrapper
    ++ * This is the _actual_ implementation of #initialize - the Ruby wrapper
     + * handles keyword-argument handling then calls this method.
       */
     -static VALUE init(VALUE self, VALUE size)
    @@ ext/raindrops/raindrops.c: static VALUE init(VALUE self, VALUE size)
     -	r->drops = mmap(NULL, tmp,
     -	                PROT_READ|PROT_WRITE, MAP_ANON|MAP_SHARED, -1, 0);
     +	if (RTEST(r->io)) {
    -+		rb_funcall(r->io, rb_intern("truncate"), 1, SIZET2NUM(tmp));
     +		int fd = NUM2INT(rb_funcall(r->io, rb_intern("fileno"), 0));
    ++		rb_funcall(r->io, rb_intern("truncate"), 1, SIZET2NUM(tmp));
     +		r->drops = mmap(NULL, tmp,
     +				PROT_READ|PROT_WRITE, MAP_SHARED, fd, 0);
     +	} else {
     +		r->drops = mmap(NULL, tmp,
    -+				PROT_READ|PROT_WRITE, MAP_ANON|MAP_SHARED, -1, 0);
    ++				PROT_READ|PROT_WRITE, MAP_ANON|MAP_SHARED,
    ++				-1, 0);
     +	}
      	if (r->drops == MAP_FAILED) {
      		int err = errno;
    @@ ext/raindrops/raindrops.c: retry:
      	}
      	r->pid = getpid();
      
    -+	if (RTEST(zero)) {
    ++	if (RTEST(zero))
     +		memset(r->drops, 0, tmp);
    -+	}
     +
      	return self;
      }

Re: [PATCH v2] Allow Raindrops objects to be backed by a file

[KJ Tsanaktsidis]

On Sat, Nov 27, 2021 at 5:34 AM Eric Wong <e@80x24.org> wrote:
>
> <snip>
>
> Note that applies to other projects, too: instead of a solid
> "-------" line, using a "----8<----" line as below
> (without indentation) for --scissors:
>
> <prose not intended for a git commit message>
>
> ----8<----
> Subject: [PATCH] Allow Raindrops objects to be backed by a file
>
> <rest of the commit message + patch...>
>
> The above allows maintainers to use `git am --scissors' to apply
> a patch while ignoring any text above the "----8<----" line.

Ah, today I learned, thanks for the tip.

> <snip>
>
> s/theat/that/ (I can squash this in on my end)

Uh, sorry about that. I could have sworn I did a pass looking for
typos. Apparently I missed a few.

> <snip>
> > - * Duplicates and snapshots the current state of a Raindrops object.
> > + * Duplicates and snapshots the current state of a Raindrops object. Even
> > + * if the given Raindrops object is backed by a file, the copy will be backed
> > + * by independent, anonymously mapped memory.
> > */
> > static VALUE init_copy(VALUE dest, VALUE source)
> > {
> > struct raindrops *dst = DATA_PTR(dest);
> > struct raindrops *src = get(source);
> >
> > - init(dest, SIZET2NUM(src->size));
> > + init_cimpl(dest, SIZET2NUM(src->size), Qnil, Qfalse);
> > memcpy(dst->drops, src->drops, raindrop_size * src->size);
>
> Switching from file-backed to anonymous seems strange to me,
> here, but I'm not sure of a better solution that's compatible
> with Object#dup ... I also don't know if anybody uses
> Raindrops#dup, either. So if you're satisfied with this
> behavior, that's good enough for me :>

I did wonder what I should do here. Could _also_ dup the file descriptor,
but then the raindrops will still share memory, which seems like the
opposite of what #dup should do. If someone wants to do something
different I guess the copy-constructor-ish initialize method could be
exposed for this purpose.

> Also, was there anything else you had planned? (e.g. for memfd)
> Otherwise, I'm thinking of tagging and releasing a new version
> in a week or so (at least before Ruby 3.1 on Dec 25)

Nope, this was it. Memfd is exactly equivalent to just using an
unlinked tempfile on tmpfs so doing it this way will suit our needs
nicely. If anybody else desperately wants to use memfd here I don't
think there's much more to it than

  IO.new syscall(319, "file_name", 0)

Syscall number is even the same on x86_64 and arm64 too!

Anyway, thanks again for your help with this, and thanks also for
bearing with my very rusty git email workflow :)

KJ

Re: [PATCH v2] Allow Raindrops objects to be backed by a file

[Eric Wong]

KJ Tsanaktsidis <ktsanaktsidis@zendesk.com> wrote:
> On Sat, Nov 27, 2021 at 5:34 AM Eric Wong <e@80x24.org> wrote:
> > s/theat/that/ (I can squash this in on my end)
> 
> Uh, sorry about that. I could have sworn I did a pass looking for
> typos. Apparently I missed a few.

No worries, I slip plenty of typos into stuff I've pushed out :x

> > > static VALUE init_copy(VALUE dest, VALUE source)
> >
> > Switching from file-backed to anonymous seems strange to me,
> > here, but I'm not sure of a better solution that's compatible
> > with Object#dup ... I also don't know if anybody uses
> > Raindrops#dup, either. So if you're satisfied with this
> > behavior, that's good enough for me :>
> 
> I did wonder what I should do here. Could _also_ dup the file descriptor,
> but then the raindrops will still share memory, which seems like the
> opposite of what #dup should do. If someone wants to do something
> different I guess the copy-constructor-ish initialize method could be
> exposed for this purpose.

Agreed.  We'll just leave things as-is for now; the API doesn't
have many direct users.

> Nope, this was it. Memfd is exactly equivalent to just using an
> unlinked tempfile on tmpfs so doing it this way will suit our needs
> nicely. If anybody else desperately wants to use memfd here I don't
> think there's much more to it than
> 
>   IO.new syscall(319, "file_name", 0)

I seem to recall there was some talk in the past about deprecating
the "syscall" method from Ruby.  But Fiddle should work, too.

> Anyway, thanks again for your help with this, and thanks also for
> bearing with my very rusty git email workflow :)

No problem, pushed to https://yhbt.net/raindrops.git as commit
18ee38af90b527f6 (Allow Raindrops objects to be backed by a file, 2021-11-25)

Hopefully some work I've done elsewhere can get more folks using
centralization-resistant workflows and tools.

Anyways, I'll tag a release sometime in the next few days.