From 688270e6c274bcb041fffe58f5af2951a3b22122 Mon Sep 17 00:00:00 2001 From: akash-akya Date: Sat, 28 Dec 2024 17:06:45 +0530 Subject: [PATCH 1/2] Enable error logging when in `DEBUG` mode --- c_src/utils.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/c_src/utils.c b/c_src/utils.c index 6583cc7..269ce43 100644 --- a/c_src/utils.c +++ b/c_src/utils.c @@ -127,7 +127,12 @@ int utils_init(ErlNifEnv *env, const char *log_level) { } else if (strcmp(log_level, "error") == 0) { VIX_LOG_LEVEL = VIX_LOG_LEVEL_ERROR; } else { +#ifdef DEBUG + // default to ERROR if we are running in debug mode + VIX_LOG_LEVEL = VIX_LOG_LEVEL_ERROR; +#else VIX_LOG_LEVEL = VIX_LOG_LEVEL_NONE; +#endif } if (VIX_LOG_LEVEL == VIX_LOG_LEVEL_WARNING || From eb0991b38a7de1819ae37626e4be1814707430cd Mon Sep 17 00:00:00 2001 From: akash-akya Date: Sat, 28 Dec 2024 17:05:54 +0530 Subject: [PATCH 2/2] Do resource cleanup on a dirty scheduler --- c_src/g_object/g_boxed.c | 50 ++++++++++++++++++++++++-- c_src/g_object/g_boxed.h | 3 ++ c_src/g_object/g_object.c | 75 ++++++++++++++++++++++++++++++++++++--- c_src/g_object/g_object.h | 3 ++ c_src/utils.c | 28 +++++++++++++++ c_src/utils.h | 3 ++ c_src/vix.c | 2 ++ lib/vix/nif.ex | 58 ++++++++++++++++++++++++++++++ test/test_helper.exs | 1 + 9 files changed, 216 insertions(+), 7 deletions(-) diff --git a/c_src/g_object/g_boxed.c b/c_src/g_object/g_boxed.c index a9b8845..346a6b3 100644 --- a/c_src/g_object/g_boxed.c +++ b/c_src/g_object/g_boxed.c @@ -7,6 +7,8 @@ ErlNifResourceType *G_BOXED_RT; +static ERL_NIF_TERM ATOM_UNREF_GBOXED; + bool erl_term_to_g_boxed(ErlNifEnv *env, ERL_NIF_TERM term, gpointer *ptr) { GBoxedResource *boxed_r = NULL; @@ -29,6 +31,26 @@ bool erl_term_boxed_type(ErlNifEnv *env, ERL_NIF_TERM term, GType *type) { return false; } +ERL_NIF_TERM nif_g_boxed_unref(ErlNifEnv *env, int argc, + const ERL_NIF_TERM argv[]) { + ASSERT_ARGC(argc, 1); + + GBoxedResource *gboxed_r = NULL; + + if (!enif_get_resource(env, argv[0], G_BOXED_RT, (void **)&gboxed_r)) { + // This should never happen, since g_boxed_unref is an internal call + return ATOM_ERROR; + } + + g_boxed_free(gboxed_r->boxed_type, gboxed_r->boxed_ptr); + + gboxed_r->boxed_ptr = NULL; + + debug("GBoxed unref"); + + return ATOM_OK; +} + ERL_NIF_TERM boxed_to_erl_term(ErlNifEnv *env, gpointer ptr, GType type) { ERL_NIF_TERM term; GBoxedResource *boxed_r; @@ -47,9 +69,29 @@ ERL_NIF_TERM boxed_to_erl_term(ErlNifEnv *env, gpointer ptr, GType type) { } static void g_boxed_dtor(ErlNifEnv *env, void *obj) { - GBoxedResource *boxed_r = (GBoxedResource *)obj; - g_boxed_free(boxed_r->boxed_type, boxed_r->boxed_ptr); - debug("GBoxedResource dtor"); + GBoxedResource *orig_boxed_r = (GBoxedResource *)obj; + + /* + * Safely unref objects using the janitor process. + * See g_object_dtor() for details + */ + if (orig_boxed_r->boxed_ptr != NULL) { + GBoxedResource *temp_gboxed_r = NULL; + ERL_NIF_TERM temp_term; + + temp_gboxed_r = enif_alloc_resource(G_BOXED_RT, sizeof(GBoxedResource)); + temp_gboxed_r->boxed_ptr = orig_boxed_r->boxed_ptr; + temp_gboxed_r->boxed_type = orig_boxed_r->boxed_type; + + temp_term = enif_make_resource(env, temp_gboxed_r); + enif_release_resource(temp_gboxed_r); + + send_to_janitor(env, ATOM_UNREF_GBOXED, temp_term); + + debug("GBoxedResource is sent to janitor process"); + } else { + debug("GBoxedResource is already unset"); + } } int nif_g_boxed_init(ErlNifEnv *env) { @@ -62,5 +104,7 @@ int nif_g_boxed_init(ErlNifEnv *env) { return 1; } + ATOM_UNREF_GBOXED = make_atom(env, "unref_gboxed"); + return 0; } diff --git a/c_src/g_object/g_boxed.h b/c_src/g_object/g_boxed.h index 63354f3..eeb45cd 100644 --- a/c_src/g_object/g_boxed.h +++ b/c_src/g_object/g_boxed.h @@ -16,6 +16,9 @@ bool erl_term_to_g_boxed(ErlNifEnv *env, ERL_NIF_TERM term, gpointer *ptr); bool erl_term_boxed_type(ErlNifEnv *env, ERL_NIF_TERM term, GType *type); +ERL_NIF_TERM nif_g_boxed_unref(ErlNifEnv *env, int argc, + const ERL_NIF_TERM argv[]); + ERL_NIF_TERM boxed_to_erl_term(ErlNifEnv *env, gpointer ptr, GType type); int nif_g_boxed_init(ErlNifEnv *env); diff --git a/c_src/g_object/g_object.c b/c_src/g_object/g_object.c index 32b7b0f..a08456f 100644 --- a/c_src/g_object/g_object.c +++ b/c_src/g_object/g_object.c @@ -6,7 +6,9 @@ ErlNifResourceType *G_OBJECT_RT; -// Ownership is traferred to beam resource, `obj` must *not* be freed +static ERL_NIF_TERM ATOM_UNREF_GOBJECT; + +// Ownership is transferred to beam, `obj` must *not* be freed // by the caller ERL_NIF_TERM g_object_to_erl_term(ErlNifEnv *env, GObject *obj) { ERL_NIF_TERM term; @@ -36,6 +38,25 @@ ERL_NIF_TERM nif_g_object_type_name(ErlNifEnv *env, int argc, return make_binary(env, G_OBJECT_TYPE_NAME(obj)); } +ERL_NIF_TERM nif_g_object_unref(ErlNifEnv *env, int argc, + const ERL_NIF_TERM argv[]) { + ASSERT_ARGC(argc, 1); + + GObjectResource *gobject_r = NULL; + + if (!enif_get_resource(env, argv[0], G_OBJECT_RT, (void **)&gobject_r)) { + // This should never happen, since g_object_unref is an internal call + return ATOM_ERROR; + } + + g_object_unref(gobject_r->obj); + gobject_r->obj = NULL; + + debug("GObject unref"); + + return ATOM_OK; +} + bool erl_term_to_g_object(ErlNifEnv *env, ERL_NIF_TERM term, GObject **obj) { GObjectResource *gobject_r = NULL; if (enif_get_resource(env, term, G_OBJECT_RT, (void **)&gobject_r)) { @@ -46,9 +67,53 @@ bool erl_term_to_g_object(ErlNifEnv *env, ERL_NIF_TERM term, GObject **obj) { } static void g_object_dtor(ErlNifEnv *env, void *ptr) { - GObjectResource *gobject_r = (GObjectResource *)ptr; - g_object_unref(gobject_r->obj); - debug("GObjectResource dtor"); + GObjectResource *orig_gobject_r = (GObjectResource *)ptr; + + /** + * The resource destructor is executed inside a normal scheduler instead of a + * dirty scheduler, which can cause issues if the code is time-consuming. + * See: https://erlangforums.com/t/4290 + * + * To address this, we avoid performing time-consuming work in the destructor + * and offload it to a janitor process. The Janitor process then calls the + * time-consuming cleanup NIF code on a dirty scheduler. Since Beam + * deallocates the resource at the end of the `dtor` call, we must create a + * new resource term to pass the object to the janitor process. + * + * Resources can be of two types: + * + * 1. Normal Resource: Constructed during normal operations; the pointer to + * the object is never NULL in this case. + * + * 2. Internal Resource: Constructed within the `dtor` of a normal resource + * solely for cleanup purposes and not for image processing operations. The + * pointer to the object will be NULL after cleanup. + * + * Currently, we use this length approach for all `g_object` and + * `g_boxed` objects, including smaller types like `VipsArray` of + * integers or doubles. For these smaller objects, it might be more + * efficient to skip certain steps. However, we are deferring the + * implementation of such special cases to keep the code simple for + * now. + * + */ + if (orig_gobject_r->obj != NULL) { + GObjectResource *temp_gobject_r = NULL; + ERL_NIF_TERM temp_term; + + /* Create temporary internal resource for the cleanup */ + temp_gobject_r = enif_alloc_resource(G_OBJECT_RT, sizeof(GObjectResource)); + temp_gobject_r->obj = orig_gobject_r->obj; + + temp_term = enif_make_resource(env, temp_gobject_r); + enif_release_resource(temp_gobject_r); + send_to_janitor(env, ATOM_UNREF_GOBJECT, temp_term); + debug("GObjectResource is sent to janitor process"); + } else { + debug("GObjectResource is already unset"); + } + + return; } int nif_g_object_init(ErlNifEnv *env) { @@ -61,5 +126,7 @@ int nif_g_object_init(ErlNifEnv *env) { return 1; } + ATOM_UNREF_GOBJECT = make_atom(env, "unref_gobject"); + return 0; } diff --git a/c_src/g_object/g_object.h b/c_src/g_object/g_object.h index f5518b4..3175554 100644 --- a/c_src/g_object/g_object.h +++ b/c_src/g_object/g_object.h @@ -16,6 +16,9 @@ ERL_NIF_TERM g_object_to_erl_term(ErlNifEnv *env, GObject *obj); ERL_NIF_TERM nif_g_object_type_name(ErlNifEnv *env, int argc, const ERL_NIF_TERM argv[]); +ERL_NIF_TERM nif_g_object_unref(ErlNifEnv *env, int argc, + const ERL_NIF_TERM argv[]); + bool erl_term_to_g_object(ErlNifEnv *env, ERL_NIF_TERM term, GObject **obj); int nif_g_object_init(ErlNifEnv *env); diff --git a/c_src/utils.c b/c_src/utils.c index 269ce43..e88e592 100644 --- a/c_src/utils.c +++ b/c_src/utils.c @@ -19,6 +19,12 @@ ERL_NIF_TERM ATOM_NULL_VALUE; ERL_NIF_TERM ATOM_UNDEFINED; ERL_NIF_TERM ATOM_EAGAIN; +/** + * Name of the process responsible for cleanup, we send resources + * requiring cleanup to this process. + */ +static ERL_NIF_TERM VIX_JANITOR_PROCESS_NAME; + const guint VIX_LOG_LEVEL_NONE = 0; const guint VIX_LOG_LEVEL_WARNING = 1; const guint VIX_LOG_LEVEL_ERROR = 2; @@ -102,6 +108,26 @@ VixResult vix_result(ERL_NIF_TERM term) { return (VixResult){.is_success = true, .result = term}; } +void send_to_janitor(ErlNifEnv *env, ERL_NIF_TERM label, + ERL_NIF_TERM resource_term) { + ErlNifPid pid; + + /* Currently there is no way to raise error when any of the + condition fail. Realistically this should never fail */ + if (!enif_whereis_pid(env, VIX_JANITOR_PROCESS_NAME, &pid)) { + error("Failed to get pid for vix janitor process"); + return; + } + + if (!enif_send(env, &pid, NULL, + enif_make_tuple2(env, label, resource_term))) { + error("Failed to send unref msg to vix janitor"); + return; + } + + return; +} + static void vix_binary_dtor(ErlNifEnv *env, void *ptr) { VixBinaryResource *vix_bin_r = (VixBinaryResource *)ptr; g_free(vix_bin_r->data); @@ -118,6 +144,8 @@ int utils_init(ErlNifEnv *env, const char *log_level) { ATOM_UNDEFINED = make_atom(env, "undefined"); ATOM_EAGAIN = make_atom(env, "eagain"); + VIX_JANITOR_PROCESS_NAME = make_atom(env, "Elixir.Vix.Nif.Janitor"); + VIX_BINARY_RT = enif_open_resource_type( env, NULL, "vix_binary_resource", (ErlNifResourceDtor *)vix_binary_dtor, ERL_NIF_RT_CREATE | ERL_NIF_RT_TAKEOVER, NULL); diff --git a/c_src/utils.h b/c_src/utils.h index eebf8a0..e6371f8 100644 --- a/c_src/utils.h +++ b/c_src/utils.h @@ -124,4 +124,7 @@ void notify_consumed_timeslice(ErlNifEnv *env, ErlNifTime start, ERL_NIF_TERM to_binary_term(ErlNifEnv *env, void *data, size_t size); +void send_to_janitor(ErlNifEnv *env, ERL_NIF_TERM label, + ERL_NIF_TERM resource_term); + #endif diff --git a/c_src/vix.c b/c_src/vix.c index a097db9..c3b7b3a 100644 --- a/c_src/vix.c +++ b/c_src/vix.c @@ -68,6 +68,7 @@ static int on_load(ErlNifEnv *env, void **priv, ERL_NIF_TERM load_info) { static ErlNifFunc nif_funcs[] = { /* GObject */ {"nif_g_object_type_name", 1, nif_g_object_type_name, 0}, + {"nif_g_object_unref", 1, nif_g_object_unref, ERL_NIF_DIRTY_JOB_CPU_BOUND}, /* GType */ {"nif_g_type_from_instance", 1, nif_g_type_from_instance, 0}, @@ -151,6 +152,7 @@ static ErlNifFunc nif_funcs[] = { {"nif_vips_blob_to_erl_binary", 1, nif_vips_blob_to_erl_binary, 0}, {"nif_vips_ref_string_to_erl_binary", 1, nif_vips_ref_string_to_erl_binary, 0}, + {"nif_g_boxed_unref", 1, nif_g_boxed_unref, ERL_NIF_DIRTY_JOB_CPU_BOUND}, /* VipsForeign */ {"nif_foreign_find_load", 1, nif_foreign_find_load, 0}, diff --git a/lib/vix/nif.ex b/lib/vix/nif.ex index 739d342..396a322 100644 --- a/lib/vix/nif.ex +++ b/lib/vix/nif.ex @@ -2,7 +2,59 @@ defmodule Vix.Nif do @moduledoc false @on_load :load_nifs + defmodule Janitor do + @moduledoc false + use GenServer + + # Singleton process to safely cleanup native resources + alias __MODULE__ + + require Logger + + def start do + GenServer.start(Janitor, nil, name: Janitor) + end + + ## Callbacks + + @impl true + def init(nil) do + {:ok, nil} + end + + @allowd_types [:unref_gobject, :unref_gboxed] + + @impl true + def handle_info({type, term}, nil) when type in @allowd_types do + # Use a dedicated process to prevent blocking the Janitor + # singleton process. The task process is not linked to the + # parent to ensure that errors do not cause the Janitor to + # crash. + _ = Task.start(Janitor, :unref, [type, term]) + {:noreply, nil} + end + + @doc false + @spec unref(atom, reference()) :: :ok + def unref(type, term) do + case type do + :unref_gboxed -> + :ok = Vix.Nif.nif_g_boxed_unref(term) + + :unref_gobject -> + :ok = Vix.Nif.nif_g_object_unref(term) + end + end + end + def load_nifs do + # must be started outside the supervision tree since the process + # that calls `load_nifs` will exit eventually. + case Vix.Nif.Janitor.start() do + {:ok, _pid} -> :ok + {:error, {:already_started, _pid}} -> :ok + end + nif_path = :filename.join(:code.priv_dir(:vix), "vix") :erlang.load_nif(nif_path, load_config()) end @@ -11,6 +63,9 @@ defmodule Vix.Nif do def nif_g_object_type_name(_obj), do: :erlang.nif_error(:nif_library_not_loaded) + def nif_g_object_unref(_obj), + do: :erlang.nif_error(:nif_library_not_loaded) + # GType def nif_g_type_from_instance(_instance), do: :erlang.nif_error(:nif_library_not_loaded) @@ -173,6 +228,9 @@ defmodule Vix.Nif do def nif_vips_ref_string_to_erl_binary(_vips_blob), do: :erlang.nif_error(:nif_library_not_loaded) + def nif_g_boxed_unref(_obj), + do: :erlang.nif_error(:nif_library_not_loaded) + # VipsForeign def nif_foreign_find_load_buffer(_binary), do: :erlang.nif_error(:nif_library_not_loaded) diff --git a/test/test_helper.exs b/test/test_helper.exs index 869559e..194f282 100644 --- a/test/test_helper.exs +++ b/test/test_helper.exs @@ -1 +1,2 @@ +Logger.configure(level: :warning) ExUnit.start()