Review response: simplify loop, separate copying function.

1 files changed, 51 insertions, 31 deletions

diff --git a/runtime/weak.c b/runtime/weak.c
index 90b3d6b5bd..ee72f635db 100644
--- a/runtime/weak.c
+++ b/runtime/weak.c
@@ -275,17 +275,50 @@ CAMLprim value caml_weak_get (value ar, value n)
   return caml_ephe_get_key(ar, n);
 }
 
+/* Copy the contents of an object from `from` to `to` (which is
+ * already allocated and has the necessary header word). Darken
+ * any pointer fields. */
+
+static void ephe_copy_and_darken(value from, value to)
+{
+  mlsize_t i = 0; /* size of non-scannable prefix */
+
+  CAMLassert(Is_block(from));
+  CAMLassert(Is_block(to));
+  CAMLassert(Tag_val(from) == Tag_val(to));
+  CAMLassert(Tag_val(from) != Infix_tag);
+  CAMLassert(Wosize_val(from) == Wosize_val(to));
+
+  if (Tag_val(from) > No_scan_tag) {
+    i = Wosize_val(to);
+  }
+  else if (Tag_val(from) == Closure_tag) {
+    i = Start_env_closinfo(Closinfo_val(from));
+  }
+
+  /* Copy non-scannable prefix */
+  memcpy (Bp_val(to), Bp_val(from), Bsize_wsize(i));
+
+  /* Copy and darken scannable fields */
+  caml_domain_state* domain_state = Caml_state;
+  while (i < Wosize_val(to)) {
+    value field = Field(from, i);
+    caml_darken (domain_state, field, 0);
+    Store_field(to, i, field);
+    ++ i;
+  }
+}
+
 static value ephe_get_field_copy (value e, mlsize_t offset)
 {
   CAMLparam1 (e);
-  CAMLlocal4 (res, val, copy, field);
-  mlsize_t i = 0, infix_offs = 0;
-  tag_t copy_tag = 0;
-  mlsize_t copy_size = 0;
-  mlsize_t copy_offs = 0;
+  CAMLlocal3 (res, val, copy);
+  mlsize_t infix_offs = 0;
 
+  copy = Val_unit;
   /* Loop in case allocating the copy triggers a GC which modifies the
-   * ephemeron or the value. */
+   * ephemeron or the value. In the common case, we go around this
+   * loop 1.5 times. */
   while(true) {
     clean_field(e, offset);
     val = Field(e, offset);
@@ -294,6 +327,7 @@ static value ephe_get_field_copy (value e, mlsize_t offset)
       res = Val_none;
       goto out;
     }
+    infix_offs = 0;
 
     /* Don't copy immediates or custom blocks #7279 */
     if (!Is_block(val) || Tag_val(val) == Custom_tag) {
@@ -306,38 +340,24 @@ static value ephe_get_field_copy (value e, mlsize_t offset)
       val -= infix_offs;
     }
 
-    if ((Tag_val(val) == copy_tag) &&
-        (Wosize_val(val) == copy_size) &&
-        (infix_offs == copy_offs))
+    if (copy != Val_unit &&
+        (Tag_val(val) == Tag_val(copy)) &&
+        (Wosize_val(val) == Wosize_val(copy))) {
+      /* The copy we allocated (on a previous iteration) is large
+       * enough and has the right header bits for us to copy the
+       * contents of val into it. Note that we don't care whether val
+       * has changed since we allocated copy. */
       break;
+    }
 
     /* This allocation could provoke a GC, which could change the
-     * header or size of val (e.g. in a finalizer). If it does, go
-     * around the loop and try again. */
+       * header or size of val (e.g. in a finalizer). So we go around
+       * the loop to read val again. */
     copy = caml_alloc (Wosize_val(val), Tag_val(val));
-    copy_size = Wosize_val(copy);
-    copy_tag = Tag_val(copy);
-    copy_offs = infix_offs;
     val = Val_unit;
   }
 
-  /* Copy non-scannable prefix */
-  if (Tag_val(val) > No_scan_tag) {
-    i = Wosize_val(copy);
-  }
-  else if (Tag_val(val) == Closure_tag) {
-    i = Start_env_closinfo(Closinfo_val(val));
-  }
-  memcpy (Bp_val(copy), Bp_val(val), Bsize_wsize(i));
-
-  /* Copy and darken scannable fields */
-  caml_domain_state* domain_state = Caml_state;
-  while (i < Wosize_val(copy)) {
-    field = Field(val, i);
-    caml_darken (domain_state, field, 0);
-    Store_field(copy, i, field);
-    ++ i;
-  }
+  ephe_copy_and_darken(val, copy);
 
 some:
   res = caml_alloc_some(copy + infix_offs);