Merge pull request #191 from akash-akya/dev

Do resource cleanup on a dirty scheduler

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;
 }

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);

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;
 }

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);

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);
@@ -127,7 +155,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 ||

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

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},

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)

test/test_helper.exs

@@ -1 +1,2 @@
+Logger.configure(level: :warning)
 ExUnit.start()