From b456869977b76d933fe87f0df7db3c72f74d0a4e Mon Sep 17 00:00:00 2001 From: Ashe Connor Date: Wed, 7 Feb 2018 14:25:53 +1100 Subject: [PATCH 1/4] defer free to GC --- ext/version_sorter/version_sorter.c | 39 +++++++++++++++++++++++------ 1 file changed, 31 insertions(+), 8 deletions(-) diff --git a/ext/version_sorter/version_sorter.c b/ext/version_sorter/version_sorter.c index d5fc691..2365482 100644 --- a/ext/version_sorter/version_sorter.c +++ b/ext/version_sorter/version_sorter.c @@ -171,10 +171,27 @@ parse_version_number(const char *string) return version; } +static VALUE rb_cSortContext; + +struct sort_context { + struct version_number **versions; + long length; +}; + +static void sort_context_free(void *p) { + struct sort_context *context = p; + long i; + + for (i = 0; i < context->length; ++i) { + xfree(context->versions[i]); + } + xfree(context->versions); + xfree(context); +} + static VALUE rb_version_sort_1(VALUE rb_self, VALUE rb_version_array, compare_callback_t cmp) { - struct version_number **versions; long length, i; VALUE *rb_version_ptr; @@ -184,7 +201,12 @@ rb_version_sort_1(VALUE rb_self, VALUE rb_version_array, compare_callback_t cmp) if (!length) return rb_ary_new(); - versions = xcalloc(length, sizeof(struct version_number *)); + struct sort_context *context; + context = ALLOC(struct sort_context); + context->length = length; + context->versions = xcalloc(length, sizeof(struct version_number *)); + + VALUE data = Data_Wrap_Struct(rb_cSortContext, NULL, sort_context_free, context); for (i = 0; i < length; ++i) { VALUE rb_version, rb_version_string; @@ -195,18 +217,17 @@ rb_version_sort_1(VALUE rb_self, VALUE rb_version_array, compare_callback_t cmp) else rb_version_string = rb_version; - versions[i] = parse_version_number(StringValueCStr(rb_version_string)); - versions[i]->rb_version = rb_version; + context->versions[i] = parse_version_number(StringValueCStr(rb_version_string)); + context->versions[i]->rb_version = rb_version; } - qsort(versions, length, sizeof(struct version_number *), cmp); + qsort(context->versions, length, sizeof(struct version_number *), cmp); rb_version_ptr = RARRAY_PTR(rb_version_array); for (i = 0; i < length; ++i) { - rb_version_ptr[i] = versions[i]->rb_version; - xfree(versions[i]); + rb_version_ptr[i] = context->versions[i]->rb_version; } - xfree(versions); + return rb_version_array; } @@ -256,4 +277,6 @@ void Init_version_sorter(void) rb_define_module_function(rb_mVersionSorter, "sort!", rb_version_sort_bang, 1); rb_define_module_function(rb_mVersionSorter, "rsort!", rb_version_sort_r_bang, 1); rb_define_module_function(rb_mVersionSorter, "compare", rb_version_compare, 2); + + rb_cSortContext = rb_define_class_under(rb_mVersionSorter, "SortContext", rb_cObject); } From c8307a67cd9784e3670a6b360b02d0f3acc750b6 Mon Sep 17 00:00:00 2001 From: Ashe Connor Date: Wed, 7 Feb 2018 14:30:49 +1100 Subject: [PATCH 2/4] protect version --- ext/version_sorter/version_sorter.c | 75 ++++++++++++++++------------- 1 file changed, 42 insertions(+), 33 deletions(-) diff --git a/ext/version_sorter/version_sorter.c b/ext/version_sorter/version_sorter.c index 2365482..70b5b23 100644 --- a/ext/version_sorter/version_sorter.c +++ b/ext/version_sorter/version_sorter.c @@ -171,47 +171,25 @@ parse_version_number(const char *string) return version; } -static VALUE rb_cSortContext; - struct sort_context { + VALUE rb_self; + VALUE rb_version_array; + compare_callback_t *cmp; struct version_number **versions; - long length; }; -static void sort_context_free(void *p) { - struct sort_context *context = p; - long i; - - for (i = 0; i < context->length; ++i) { - xfree(context->versions[i]); - } - xfree(context->versions); - xfree(context); -} - static VALUE -rb_version_sort_1(VALUE rb_self, VALUE rb_version_array, compare_callback_t cmp) +rb_version_sort_1_cb(VALUE arg) { + struct sort_context *context = (struct sort_context *)arg; long length, i; VALUE *rb_version_ptr; - Check_Type(rb_version_array, T_ARRAY); - - length = RARRAY_LEN(rb_version_array); - if (!length) - return rb_ary_new(); - - struct sort_context *context; - context = ALLOC(struct sort_context); - context->length = length; - context->versions = xcalloc(length, sizeof(struct version_number *)); - - VALUE data = Data_Wrap_Struct(rb_cSortContext, NULL, sort_context_free, context); - + length = RARRAY_LEN(context->rb_version_array); for (i = 0; i < length; ++i) { VALUE rb_version, rb_version_string; - rb_version = rb_ary_entry(rb_version_array, i); + rb_version = rb_ary_entry(context->rb_version_array, i); if (rb_block_given_p()) rb_version_string = rb_yield(rb_version); else @@ -221,13 +199,46 @@ rb_version_sort_1(VALUE rb_self, VALUE rb_version_array, compare_callback_t cmp) context->versions[i]->rb_version = rb_version; } - qsort(context->versions, length, sizeof(struct version_number *), cmp); - rb_version_ptr = RARRAY_PTR(rb_version_array); + qsort(context->versions, length, sizeof(struct version_number *), context->cmp); + rb_version_ptr = RARRAY_PTR(context->rb_version_array); for (i = 0; i < length; ++i) { rb_version_ptr[i] = context->versions[i]->rb_version; } + return context->rb_version_array; +} + +static VALUE +rb_version_sort_1(VALUE rb_self, VALUE rb_version_array, compare_callback_t cmp) +{ + long length, i; + int exception; + + Check_Type(rb_version_array, T_ARRAY); + + length = RARRAY_LEN(rb_version_array); + if (!length) + return rb_ary_new(); + + struct sort_context context = { + rb_self, + rb_version_array, + cmp, + xcalloc(length, sizeof(struct version_number *)), + }; + + VALUE result = rb_protect(rb_version_sort_1_cb, (VALUE)&context, &exception); + + for (i = 0; i < length; ++i) { + xfree(context.versions[i]); + } + xfree(context.versions); + + if (exception) { + rb_jump_tag(exception); + } + return rb_version_array; } @@ -277,6 +288,4 @@ void Init_version_sorter(void) rb_define_module_function(rb_mVersionSorter, "sort!", rb_version_sort_bang, 1); rb_define_module_function(rb_mVersionSorter, "rsort!", rb_version_sort_r_bang, 1); rb_define_module_function(rb_mVersionSorter, "compare", rb_version_compare, 2); - - rb_cSortContext = rb_define_class_under(rb_mVersionSorter, "SortContext", rb_cObject); } From b7ccd8c02e5dcb47280c84a55bfab263e2dcf838 Mon Sep 17 00:00:00 2001 From: Ashe Connor Date: Wed, 7 Feb 2018 14:34:32 +1100 Subject: [PATCH 3/4] return the result of cb --- ext/version_sorter/version_sorter.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ext/version_sorter/version_sorter.c b/ext/version_sorter/version_sorter.c index 70b5b23..c9e3849 100644 --- a/ext/version_sorter/version_sorter.c +++ b/ext/version_sorter/version_sorter.c @@ -239,7 +239,7 @@ rb_version_sort_1(VALUE rb_self, VALUE rb_version_array, compare_callback_t cmp) rb_jump_tag(exception); } - return rb_version_array; + return result; } static VALUE From 4b7b7b7a888dd8a844ae10e1c24f8e5882c8dc85 Mon Sep 17 00:00:00 2001 From: Phil Turnbull Date: Wed, 7 Feb 2018 14:55:18 -0500 Subject: [PATCH 4/4] Free memory in rb_version_compare when exception is thrown --- ext/version_sorter/version_sorter.c | 33 ++++++++++++++++++++++++----- 1 file changed, 28 insertions(+), 5 deletions(-) diff --git a/ext/version_sorter/version_sorter.c b/ext/version_sorter/version_sorter.c index c9e3849..081e3f2 100644 --- a/ext/version_sorter/version_sorter.c +++ b/ext/version_sorter/version_sorter.c @@ -266,16 +266,39 @@ rb_version_sort_r_bang(VALUE rb_self, VALUE rb_versions) return rb_version_sort_1(rb_self, rb_versions, version_compare_cb_r); } +struct compare_context { + VALUE rb_version_a, rb_version_b; + struct version_number *version_a, *version_b; +}; + +static VALUE +rb_version_compare_cb(VALUE arg) +{ + struct compare_context *context = (struct compare_context *)arg; + + context->version_a = parse_version_number(StringValueCStr(context->rb_version_a)); + context->version_b = parse_version_number(StringValueCStr(context->rb_version_b)); + + return INT2NUM(version_compare_cb(&context->version_a, &context->version_b)); +} + static VALUE rb_version_compare(VALUE rb_self, VALUE rb_version_a, VALUE rb_version_b) { - struct version_number *version_a = parse_version_number(StringValueCStr(rb_version_a)); - struct version_number *version_b = parse_version_number(StringValueCStr(rb_version_b)); + int exception; + struct compare_context context = { + rb_version_a, rb_version_b, + NULL, NULL, + }; + + VALUE result = rb_protect(rb_version_compare_cb, (VALUE)&context, &exception); - VALUE result = INT2NUM(version_compare_cb(&version_a, &version_b)); + xfree(context.version_a); + xfree(context.version_b); - xfree(version_a); - xfree(version_b); + if (exception) { + rb_jump_tag(exception); + } return result; }