Re: [PATCH 2/2] ruby: enable garbage collection using talloc
On Sat, Jun 26, 2021 at 2:54 PM David Bremner wrote: > > Felipe Contreras writes: > > >> > >> One issue to double check: in a few places we explicitely _don't_ use > >> talloc. What happens when those objects are passed to talloc_steal? > > > > Seems like talloc aborts. Are there any of such objects? > > > > At least anything where notmuch.h says "allocated by malloc and should be > freed > by the caller". > > - all of the error_message output parameters > - notmuch_database_get_config output parameter 'value' I meant objects in the context of Ruby objects: Database, Directory, FileNames, Query, Threds, Thread, Messages, Message, Tags. I think all of them correspond to a notmuch_*_t that was allocated with talloc. -- Felipe Contreras ___ notmuch mailing list -- notmuch@notmuchmail.org To unsubscribe send an email to notmuch-le...@notmuchmail.org
Re: [PATCH 2/2] ruby: enable garbage collection using talloc
Felipe Contreras writes: > > I have the test ready, the only question is how many times to run it > in a loop. 100 times takes about 10 minutes with the large corpus > size. > That sounds reasonable for a performance-test (in line with T00-new) d ___ notmuch mailing list -- notmuch@notmuchmail.org To unsubscribe send an email to notmuch-le...@notmuchmail.org
Re: [PATCH 2/2] ruby: enable garbage collection using talloc
Felipe Contreras writes: >> >> One issue to double check: in a few places we explicitely _don't_ use >> talloc. What happens when those objects are passed to talloc_steal? > > Seems like talloc aborts. Are there any of such objects? > At least anything where notmuch.h says "allocated by malloc and should be freed by the caller". - all of the error_message output parameters - notmuch_database_get_config output parameter 'value' ___ notmuch mailing list -- notmuch@notmuchmail.org To unsubscribe send an email to notmuch-le...@notmuchmail.org
Re: [PATCH 2/2] ruby: enable garbage collection using talloc
On Fri, Jun 11, 2021 at 3:54 AM David Bremner wrote: > > David Bremner writes: > > > Felipe Contreras writes: > > > >> We basically steal all the objects from their notmuch parents, therefore > >> they are completely under Ruby's gc control. > >> > >> The order at which these objects are freed does not matter any more, > >> because destroying the database does not destroy all the children > >> objects, since they belong to Ruby now. > >> > > > > I guess from a purist point of view this is a kind of layering > > violation, since the use of talloc is purportedly an internal > > implementation detail of the library. Still, I think it's a reasonable > > approach given that the ruby bindings are maintained as part of notmuch, > > and we are not very likely to abandon talloc. > > > > One issue to double check: in a few places we explicitely _don't_ use > talloc. What happens when those objects are passed to talloc_steal? Seems like talloc aborts. Are there any of such objects? -- Felipe Contreras ___ notmuch mailing list -- notmuch@notmuchmail.org To unsubscribe send an email to notmuch-le...@notmuchmail.org
Re: [PATCH 2/2] ruby: enable garbage collection using talloc
On Thu, Jun 10, 2021 at 7:46 PM David Bremner wrote: > > Felipe Contreras writes: > > > We basically steal all the objects from their notmuch parents, therefore > > they are completely under Ruby's gc control. > > > > The order at which these objects are freed does not matter any more, > > because destroying the database does not destroy all the children > > objects, since they belong to Ruby now. > > I guess from a purist point of view this is a kind of layering > violation, since the use of talloc is purportedly an internal > implementation detail of the library. Still, I think it's a reasonable > approach given that the ruby bindings are maintained as part of notmuch, > and we are not very likely to abandon talloc. Yes, some refcount API could be added to all the notmuch objects, but at least I couldn't get talloc refcount stuff work for Ruby, so more work would be needed for that. > I am OK with applying the series as is. The only sensible thing I can > think of at the moment for testing is to run something like the script > of id:20210517193915.1220114-1-felipe.contre...@gmail.com as a "time > test", so not attempt to get valgrind working, but just run it on some > decent size corpuses and see that it it does not crash or leak too much > memory to complete. I have the test ready, the only question is how many times to run it in a loop. 100 times takes about 10 minutes with the large corpus size. -- Felipe Contreras ___ notmuch mailing list -- notmuch@notmuchmail.org To unsubscribe send an email to notmuch-le...@notmuchmail.org
Re: [PATCH 2/2] ruby: enable garbage collection using talloc
David Bremner writes: > Felipe Contreras writes: > >> We basically steal all the objects from their notmuch parents, therefore >> they are completely under Ruby's gc control. >> >> The order at which these objects are freed does not matter any more, >> because destroying the database does not destroy all the children >> objects, since they belong to Ruby now. >> > > I guess from a purist point of view this is a kind of layering > violation, since the use of talloc is purportedly an internal > implementation detail of the library. Still, I think it's a reasonable > approach given that the ruby bindings are maintained as part of notmuch, > and we are not very likely to abandon talloc. > One issue to double check: in a few places we explicitely _don't_ use talloc. What happens when those objects are passed to talloc_steal? d ___ notmuch mailing list -- notmuch@notmuchmail.org To unsubscribe send an email to notmuch-le...@notmuchmail.org
Re: [PATCH 2/2] ruby: enable garbage collection using talloc
Felipe Contreras writes: > We basically steal all the objects from their notmuch parents, therefore > they are completely under Ruby's gc control. > > The order at which these objects are freed does not matter any more, > because destroying the database does not destroy all the children > objects, since they belong to Ruby now. > I guess from a purist point of view this is a kind of layering violation, since the use of talloc is purportedly an internal implementation detail of the library. Still, I think it's a reasonable approach given that the ruby bindings are maintained as part of notmuch, and we are not very likely to abandon talloc. I am OK with applying the series as is. The only sensible thing I can think of at the moment for testing is to run something like the script of id:20210517193915.1220114-1-felipe.contre...@gmail.com as a "time test", so not attempt to get valgrind working, but just run it on some decent size corpuses and see that it it does not crash or leak too much memory to complete. d ___ notmuch mailing list -- notmuch@notmuchmail.org To unsubscribe send an email to notmuch-le...@notmuchmail.org
[PATCH 2/2] ruby: enable garbage collection using talloc
We basically steal all the objects from their notmuch parents, therefore
they are completely under Ruby's gc control.
The order at which these objects are freed does not matter any more,
because destroying the database does not destroy all the children
objects, since they belong to Ruby now.
Signed-off-by: Felipe Contreras
---
bindings/ruby/database.c | 2 +-
bindings/ruby/defs.h | 11 +++
bindings/ruby/extconf.rb | 1 +
3 files changed, 9 insertions(+), 5 deletions(-)
diff --git a/bindings/ruby/database.c b/bindings/ruby/database.c
index 66100de2..3737be17 100644
--- a/bindings/ruby/database.c
+++ b/bindings/ruby/database.c
@@ -81,7 +81,7 @@ notmuch_rb_database_initialize (int argc, VALUE *argv, VALUE
self)
ret = notmuch_database_open (path, mode, );
notmuch_rb_status_raise (ret);
-DATA_PTR (self) = notmuch_rb_object_create (database);
+DATA_PTR (self) = notmuch_rb_object_create (database,
"notmuch_rb_database");
return self;
}
diff --git a/bindings/ruby/defs.h b/bindings/ruby/defs.h
index 1413eb72..5cebd5fa 100644
--- a/bindings/ruby/defs.h
+++ b/bindings/ruby/defs.h
@@ -23,6 +23,7 @@
#include
#include
+#include
extern VALUE notmuch_rb_cDatabase;
extern VALUE notmuch_rb_cDirectory;
@@ -83,7 +84,7 @@ extern const rb_data_type_t notmuch_rb_tags_type;
} while (0)
#define Data_Wrap_Notmuch_Object(klass, type, ptr) \
-TypedData_Wrap_Struct ((klass), (type), notmuch_rb_object_create ((ptr)))
+TypedData_Wrap_Struct ((klass), (type), notmuch_rb_object_create ((ptr),
"notmuch_rb_object: " __location__))
#define Data_Get_Notmuch_Database(obj, ptr) \
Data_Get_Notmuch_Object ((obj), _rb_database_type, (ptr))
@@ -117,20 +118,22 @@ typedef struct {
} notmuch_rb_object_t;
static inline void *
-notmuch_rb_object_create (void *nm_object)
+notmuch_rb_object_create (void *nm_object, const char *name)
{
-notmuch_rb_object_t *rb_wrapper = malloc (sizeof (*rb_wrapper));
+notmuch_rb_object_t *rb_wrapper = talloc_named_const (NULL, sizeof
(*rb_wrapper), name);
+
if (RB_UNLIKELY (!rb_wrapper))
return NULL;
rb_wrapper->nm_object = nm_object;
+talloc_steal (rb_wrapper, nm_object);
return rb_wrapper;
}
static inline void
notmuch_rb_object_free (void *rb_wrapper)
{
-free (rb_wrapper);
+talloc_free (rb_wrapper);
}
static inline notmuch_status_t
diff --git a/bindings/ruby/extconf.rb b/bindings/ruby/extconf.rb
index 161de5a2..d914537c 100644
--- a/bindings/ruby/extconf.rb
+++ b/bindings/ruby/extconf.rb
@@ -19,6 +19,7 @@ if not ENV['LIBNOTMUCH']
end
$LOCAL_LIBS += ENV['LIBNOTMUCH']
+$LIBS += " -ltalloc"
# Create Makefile
dir_config('notmuch')
--
2.31.1
___
notmuch mailing list -- notmuch@notmuchmail.org
To unsubscribe send an email to notmuch-le...@notmuchmail.org
