Skip to content

Commit

Permalink
class.c: descendants must not cause GC until the result array is created
Browse files Browse the repository at this point in the history
Follow up of 4282274. The previous fix
uses `rb_ary_new_from_values` to create the result array, but it may
trigger the GC.

This second try is to create the result array by `rb_ary_new_capa`
before the second iteration, and assume that `rb_ary_push` does not
trigger GC. This assumption is very fragile, so should be improved in
future.

[Bug #18282] [Feature #14394]
  • Loading branch information
mame committed Nov 10, 2021
1 parent 0d3898e commit 5c892da
Show file tree
Hide file tree
Showing 2 changed files with 21 additions and 9 deletions.
22 changes: 13 additions & 9 deletions class.c
Expand Up @@ -29,6 +29,7 @@
#include "internal/variable.h"
#include "ruby/st.h"
#include "vm_core.h"
#include "gc.h"

#define id_attached id__attached__

Expand Down Expand Up @@ -1338,7 +1339,7 @@ rb_mod_ancestors(VALUE mod)

struct subclass_traverse_data
{
VALUE *buffer;
VALUE buffer;
long count;
long maxcount;
};
Expand All @@ -1349,8 +1350,9 @@ class_descendants_recursive(VALUE klass, VALUE v)
struct subclass_traverse_data *data = (struct subclass_traverse_data *) v;

if (BUILTIN_TYPE(klass) == T_CLASS && !FL_TEST(klass, FL_SINGLETON)) {
if (data->buffer && data->count < data->maxcount) {
data->buffer[data->count] = klass;
if (data->buffer && data->count < data->maxcount && !rb_objspace_garbage_object_p(klass)) {
// assumes that this does not cause GC as long as the length does not exceed the capacity
rb_ary_push(data->buffer, klass);
}
data->count++;
}
Expand Down Expand Up @@ -1378,24 +1380,26 @@ class_descendants_recursive(VALUE klass, VALUE v)
VALUE
rb_class_descendants(VALUE klass)
{
struct subclass_traverse_data data = { NULL, 0, -1 };
struct subclass_traverse_data data = { Qfalse, 0, -1 };

// estimate the count of subclasses
rb_class_foreach_subclass(klass, class_descendants_recursive, (VALUE) &data);

// the following allocation may cause GC which may change the number of subclasses
data.buffer = ALLOC_N(VALUE, data.count);
data.buffer = rb_ary_new_capa(data.count);
data.maxcount = data.count;
data.count = 0;

size_t gc_count = rb_gc_count();

// enumerate subclasses
rb_class_foreach_subclass(klass, class_descendants_recursive, (VALUE) &data);

VALUE ary = rb_ary_new_from_values(data.count, data.buffer);

free(data.buffer);
if (gc_count != rb_gc_count()) {
rb_bug("GC must not occur during the subclass iteration of Class#descendants");
}

return ary;
return data.buffer;
}

static void
Expand Down
8 changes: 8 additions & 0 deletions test/ruby/test_class.rb
Expand Up @@ -761,4 +761,12 @@ def test_descendants_gc
100000.times { Class.new(c) }
assert(c.descendants.size <= 100000)
end

def test_descendants_gc_stress
10000.times do
c = Class.new
100.times { Class.new(c) }
assert(c.descendants.size <= 100)
end
end
end

0 comments on commit 5c892da

Please sign in to comment.