Merge pull request #1457 from svaarala/allow-markandsweep-during-fina…

…lization

Allow mark-and-sweep during finalize_list processing

RELEASES.rst

@@ -2608,9 +2608,9 @@ Planned
   for mark-and-sweep to confirm its status (GH-1427)
 
 * Rework finalizer handling: finalizer execution is now outside of refzero
-  processing and mark-and-sweep; however, mark-and-sweep is still disabled
-  while finalizers are being executed to avoid incorrect rescue decisions
-  caused by a partially processed finalize_list (GH-1427, GH-1451)
+  processing and mark-and-sweep, and mark-and-sweep (but not recursive
+  finalizer handling) is allowed during finalizer execution (GH-1427, GH-1451,
+  GH-1457)
 
 * Rework mark-and-sweep: include finalize_list in TEMPROOT marking; with
   duk_push_heapptr() allowed it's possible for application code to create

doc/memory-management.rst

@@ -1150,18 +1150,18 @@ The mark-and-sweep algorithm is as follows:
    d. No object in the "refzero" work list has been freed.
 
 9. Execute pending finalizers unless finalizer execution is prevented or an
-   earlier call site is already finalizing objects (currently mark-and-sweep
-   is not allowed during finalization, but that may change).
+   earlier call site is already finalizing objects.  Finalizer execution is
+   outside of mark-and-sweep prevention lock, so mark-and-sweep may run while
+   finalizers are being processed.  However, rescue decisions are postponed
+   until the finalize_list is empty to avoid incorrect rescue decisions caused
+   by finalize_list being treated as a reachability root.
 
 Notes:
 
 * Elements on the refzero list are considered reachability roots, as we need
   to preserve both the object itself (which happens automatically because we
-  don't sweep the refzero_list) and its children.  If the refzero list elements
-  were not considered reachability roots, their children might be swept by the
-  sweep phase.  This would be problematic for processing the objects in the
-  refzero list, regardless of whether they have a finalizer or not, as some
-  references would be dangling pointers.
+  don't sweep the refzero_list) and its children.  (This is no longer relevant
+  because refzero_list is always NULL when mark-and-sweep runs.)
 
 * Elements marked FINALIZABLE are considered reachability roots to ensure
   that their children (e.g. property values) are not swept during the
@@ -1178,12 +1178,10 @@ Notes:
     pass, including running finalizers.
 
 * Finalizers are executed after the sweep phase to ensure that finalizers
-  have as much available memory as possible.  While finalizers execute outside
-  the mark-and-sweep algorithm (since Duktape 2.1), mark-and-sweep is
-  explicitly prevented during finalization because it may cause incorrect
-  rescue/free decisions when the finalize_list is only partially processed.
-  As a result, no memory can be reclaimed while the finalize_list is being
-  processed.  This is probably a very minor issue in practice.
+  have as much available memory as possible.  Since Duktape 2.1 mark-and-sweep
+  runs outside the mark-and-sweep algorithm, and mark-and-sweep may run while
+  finalizers are being processed, with the limitation that rescue decisions
+  are postponed until finalize_list is empty.
 
 * The sweep phase is divided into two separate scans: one to adjust refcounts
   and one to actually free the objects.  If these were performed in a single
@@ -1205,23 +1203,6 @@ Notes:
     freed inline without side effects, so it's always NULL when mark-and-sweep
     runs.)
 
-Note that there is a small "hole" in the reclamation right now, when
-mark-and-sweep finalizers are used:
-
-* If a finalizer executed by mark-and-sweep removes a reference to another
-  object (not the object being finalized), causing the target object's
-  reference count to drop to zero, the object is *not* placed in the
-  "refzero" work list, as mark-and-sweep is still running.
-
-* As a result, the object will be unreachable and will not be freed by
-  the reference count algorithm, regardless of whether the object was part
-  of a reference loop.  Instead, the next mark-and-sweep will free the object.
-  If the object has a finalizer, the finalizer will be called later than
-  would be preferable.
-
-* This is not ideal but will not result in memory leaks, so it's not really
-  worth fixing right now.
-
 Interactions between reference counting and mark-and-sweep
 ==========================================================
 
@@ -1344,65 +1325,7 @@ error handling a bit easier.
 Side effects of memory management
 ---------------------------------
 
-Automatic memory management may be triggered by various operations, and has
-a wide variety of side effects which must be taken into account by calling
-code.  This affects internal code in particular, which must be very careful
-not to reference dangling pointers, deal with valstack and object property
-allocation resizes, etc.
-
-The fundamental triggers for memory management side effects are:
-
-* An attempt to ``alloc`` or ``realloc`` memory may trigger a garbage
-  collection.  A collection is triggered by an out-of-memory condition,
-  but a voluntary garbage collection also occurs periodically.  A ``free``
-  operation cannot, at the moment, trigger a collection.
-
-* An explicit request for garbage collection.
-
-* A ``DECREF`` operation which drops the target heap element reference
-  count to zero triggers the element (and possibly a bunch of other
-  elements) to be freed, and may invoke a number of finalizers.  Also,
-  a mark-and-sweep may be triggered (e.g. by finalizers or voluntarily).
-
-The following primitives do not trigger any side effects:
-
-* An ``INCREF`` operation never causes a side effect.
-
-* A ``free`` operation never causes a side effect.
-
-Because of finalizers, the side effects of a ``DECREF`` and a mark-and-sweep
-are potentially the same as running arbitrary C or Ecmascript code,
-including:
-
-* Calling (further) finalizer functions (= running arbitrary Ecmascript and C code).
-
-* Resizing object allocations, value stacks, catch stacks, call stacks, buffers,
-  object property allocations, etc.
-
-* Compacting object property allocations, abandoning array parts.
-
-* In particular:
-
-  + Any ``duk_tval`` pointers referring any value stack may be invalidated,
-    because any value stack may be resized.  Value stack indices are OK.
-
-  + Any ``duk_tval`` pointers referring any object property values may be
-    invalidated, because any property allocation may be resized.  Also,
-    any indices to object property slots may be invalidated due to
-    "compaction" which happens during a property allocation resize.
-
-  + Heap element pointers are stable, so they are never affected.
-
-The side effects can be avoided by many techniques:
-
-* Refer to value stack using a numeric index.
-
-* Make a copy of an ``duk_tval`` to a C local to ensure the value can still
-  be used after a side effect occurs.  If the value is primitive, it will
-  OK in any case.  If the value is a heap reference, the reference uses a
-  stable pointer which is OK as long as the target is still reachable.
-
-* Re-lookup object property slots after a potential side effect.
+See ``doc/side-effects.rst``.
 
 Misc notes
 ==========

doc/objects-in-code-section.rst

@@ -59,7 +59,8 @@ duk_heaphdr
   - Mark-and-sweep: cannot mark object reachable, temproot, etc.  Built-in
     strings/objects must not be marked "visited".
 
-  - No finalizer support: cannot mark finalizable, finalized.
+  - No finalizer support: cannot mark finalizable, finalized.  Also no need
+    for finalization because ROM objects don't need to be freed.
 
 * Can't update refcount.
 

doc/side-effects.rst

@@ -418,9 +418,12 @@ Mark-and-sweep
 
 * Executes finalizers after mark-and-sweep is complete (unless prevented),
   which has arbitrary code execution side effects.  Finalizer execution
-  happens outside of mark-and-sweep protection, but currently finalizer
-  execution explicitly prevents mark-and-sweep to avoid incorrect rescue/free
-  decisions when the finalize_list is only partially processed.
+  happens outside of mark-and-sweep protection, and may trigger mark-and-sweep.
+  However, when mark-and-sweep runs with finalize_list != NULL, rescue
+  decisions are postponed to avoid incorrect rescue decisions caused by
+  finalize_list being (artificially) treated as a reachability root; in
+  concrete terms, FINALIZED flags are not cleared so they'll be rechecked
+  later.
 
 Error throw
 

src-input/duk_heap.h

@@ -75,6 +75,12 @@
 /* Voluntary mark-and-sweep: triggered periodically. */
 #define DUK_MS_FLAG_VOLUNTARY                (1 << 1)
 
+/* Postpone rescue decisions for reachable objects with FINALIZED set.
+ * Used during finalize_list processing to avoid incorrect rescue
+ * decisions due to finalize_list being a reachability root.
+ */
+#define DUK_MS_FLAG_POSTPONE_RESCUE          (1 << 2)
+
 /* Don't compact objects; needed during object property table resize
  * to prevent a recursive resize.  It would suffice to protect only the
  * current object being resized, but this is not yet implemented.
@@ -344,9 +350,13 @@ struct duk_heap {
 	duk_heaphdr *refzero_list;
 #endif
 
-	/* Work list for objects to be finalized (by mark-and-sweep). */
 #if defined(DUK_USE_FINALIZER_SUPPORT)
+	/* Work list for objects to be finalized. */
 	duk_heaphdr *finalize_list;
+#if defined(DUK_USE_ASSERTIONS)
+	/* Object whose finalizer is executing right now (no nesting). */
+	duk_heaphdr *currently_finalizing;
+#endif
 #endif
 
 	/* Voluntary mark-and-sweep trigger counter.  Intentionally signed

src-input/duk_heap_alloc.c

@@ -823,6 +823,9 @@ duk_heap *duk_heap_alloc(duk_alloc_function alloc_func,
 #endif
 #if defined(DUK_USE_FINALIZER_SUPPORT)
 	res->finalize_list = NULL;
+#if defined(DUK_USE_ASSERTIONS)
+	res->currently_finalizing = NULL;
+#endif
 #endif
 	res->heap_thread = NULL;
 	res->curr_thread = NULL;

src-input/duk_heap_finalize.c

@@ -153,17 +153,13 @@ DUK_INTERNAL void duk_heap_process_finalize_list(duk_heap *heap) {
 	DUK_ASSERT(heap->pf_prevent_count == 0);
 	heap->pf_prevent_count = 1;
 
-	/* Bump ms_prevent_count to prevent mark-and-sweep while we execute
-	 * finalizers.  It's important for no mark-and-sweep passes to happen
-	 * while we process the finalize_list.  If a part of the finalize_list
-	 * has been processed and mark-and-sweep runs, it will incorrectly
-	 * consider the processed objects rescued if they are in a reference
-	 * relationship with objects still in finalize_list.  This happens
-	 * because mark-and-sweep treats the whole finalize_list as being
-	 * "reachable".
+	/* Mark-and-sweep no longer needs to be prevented when running
+	 * finalizers: mark-and-sweep skips any rescue decisions if there
+	 * are any objects in finalize_list when mark-and-sweep is entered.
+	 * This protects finalized objects from incorrect rescue decisions
+	 * caused by finalize_list being a reachability root and only
+	 * partially processed.  Freeing decisions are not postponed.
 	 */
-	heap->ms_prevent_count++;
-	DUK_ASSERT(heap->ms_prevent_count != 0);  /* Wrap. */
 
 	/* When finalizer torture is enabled, make a fake finalizer call with
 	 * maximum side effects regardless of whether finalize_list is empty.
@@ -185,10 +181,15 @@ DUK_INTERNAL void duk_heap_process_finalize_list(duk_heap *heap) {
 		DUK_ASSERT(DUK_HEAPHDR_GET_TYPE(curr) == DUK_HTYPE_OBJECT);  /* Only objects have finalizers. */
 		DUK_ASSERT(!DUK_HEAPHDR_HAS_REACHABLE(curr));
 		DUK_ASSERT(!DUK_HEAPHDR_HAS_TEMPROOT(curr));
-		DUK_ASSERT(DUK_HEAPHDR_HAS_FINALIZABLE(curr));  /* All objects on finalize_list will have this flag. */
+		DUK_ASSERT(DUK_HEAPHDR_HAS_FINALIZABLE(curr));  /* All objects on finalize_list will have this flag (except object being finalized right now). */
 		DUK_ASSERT(!DUK_HEAPHDR_HAS_FINALIZED(curr));   /* Queueing code ensures. */
 		DUK_ASSERT(!DUK_HEAPHDR_HAS_READONLY(curr));  /* ROM objects never get freed (or finalized). */
 
+#if defined(DUK_USE_ASSERTIONS)
+		DUK_ASSERT(heap->currently_finalizing == NULL);
+		heap->currently_finalizing = curr;
+#endif
+
 		/* Clear FINALIZABLE for object being finalized, so that
 		 * duk_push_heapptr() can properly ignore the object.
 		 */
@@ -296,6 +297,11 @@ DUK_INTERNAL void duk_heap_process_finalize_list(duk_heap *heap) {
 #if defined(DUK_USE_DEBUG)
 		count++;
 #endif
+
+#if defined(DUK_USE_ASSERTIONS)
+		DUK_ASSERT(heap->currently_finalizing != NULL);
+		heap->currently_finalizing = NULL;
+#endif
 	}
 
 	/* finalize_list will always be processed completely. */
@@ -309,9 +315,6 @@ DUK_INTERNAL void duk_heap_process_finalize_list(duk_heap *heap) {
 	DUK_REFZERO_CHECK_SLOW(heap->heap_thread);
 #endif
 
-	DUK_ASSERT(heap->ms_prevent_count > 0);
-	heap->ms_prevent_count--;
-
 	/* Prevent count may be bumped while finalizers run, but should always
 	 * be reliably unbumped by the time we get here.
 	 */