[API] Simplify blob API, remove lock

TODO

@@ -16,8 +16,6 @@ API issues to fix before 1.0:
 
 - Add hb-cairo glue
 
-- Fix blob, remove mutex, etc.
-
 - Add sanitize API
 
 - Add glib GBoxedType stuff

src/hb-blob.cc

@@ -28,7 +28,6 @@
 
 #include "hb-blob.h"
 #include "hb-object-private.hh"
-#include "hb-mutex-private.hh"
 
 #ifdef HAVE_SYS_MMAN_H
 #ifdef HAVE_UNISTD_H
@@ -51,15 +50,11 @@ HB_BEGIN_DECLS
 struct _hb_blob_t {
   hb_object_header_t header;
 
-  unsigned int length;
+  bool immutable;
-
-  hb_mutex_t lock;
-  /* the rest are protected by lock */
-
-  unsigned int lock_count;
-  hb_memory_mode_t mode;
 
   const char *data;
+  unsigned int length;
+  hb_memory_mode_t mode;
 
   void *user_data;
   hb_destroy_func_t destroy;
@@ -68,19 +63,19 @@ struct _hb_blob_t {
 static hb_blob_t _hb_blob_nil = {
   HB_OBJECT_HEADER_STATIC,
 
+  TRUE, /* immutable */
+
   0, /* length */
-
-  HB_MUTEX_INIT, /* lock */
-
-  0, /* lock_count */
-  HB_MEMORY_MODE_READONLY, /* mode */
-
   NULL, /* data */
+  HB_MEMORY_MODE_READONLY, /* mode */
 
   NULL, /* user_data */
   NULL  /* destroy */
 };
 
+
+static bool _try_writable (hb_blob_t *blob);
+
 static void
 _hb_blob_destroy_user_data (hb_blob_t *blob)
 {
@@ -91,13 +86,6 @@ _hb_blob_destroy_user_data (hb_blob_t *blob)
   }
 }
 
-static void
-_hb_blob_unlock_and_destroy (hb_blob_t *blob)
-{
-  hb_blob_unlock (blob);
-  hb_blob_destroy (blob);
-}
-
 hb_blob_t *
 hb_blob_create (const char        *data,
 		unsigned int       length,
@@ -113,9 +101,6 @@ hb_blob_create (const char        *data,
     return &_hb_blob_nil;
   }
 
-  hb_mutex_init (&blob->lock);
-  blob->lock_count = 0;
-
   blob->data = data;
   blob->length = length;
   blob->mode = mode;
@@ -125,7 +110,7 @@ hb_blob_create (const char        *data,
 
   if (blob->mode == HB_MEMORY_MODE_DUPLICATE) {
     blob->mode = HB_MEMORY_MODE_READONLY;
-    if (!hb_blob_try_writable (blob)) {
+    if (!_try_writable (blob)) {
       hb_blob_destroy (blob);
       return &_hb_blob_nil;
     }
@@ -140,20 +125,17 @@ hb_blob_create_sub_blob (hb_blob_t    *parent,
 			 unsigned int  length)
 {
   hb_blob_t *blob;
-  const char *pdata;
 
   if (!length || offset >= parent->length || !(blob = hb_object_create<hb_blob_t> ()))
     return &_hb_blob_nil;
 
-  pdata = hb_blob_lock (parent);
+  hb_blob_make_immutable (parent);
 
-  hb_mutex_lock (&parent->lock);
+  blob = hb_blob_create (parent->data + offset,
-  blob = hb_blob_create (pdata + offset,
 			 MIN (length, parent->length - offset),
 			 parent->mode,
 			 hb_blob_reference (parent),
-			 (hb_destroy_func_t) _hb_blob_unlock_and_destroy);
+			 (hb_destroy_func_t) hb_blob_destroy);
-  hb_mutex_unlock (&parent->lock);
 
   return blob;
 }
@@ -176,7 +158,6 @@ hb_blob_destroy (hb_blob_t *blob)
   if (!hb_object_destroy (blob)) return;
 
   _hb_blob_destroy_user_data (blob);
-  hb_mutex_free (&blob->lock);
 
   free (blob);
 }
@@ -198,6 +179,22 @@ hb_blob_get_user_data (hb_blob_t          *blob,
 }
 
 
+void
+hb_blob_make_immutable (hb_blob_t *blob)
+{
+  if (hb_object_is_inert (blob))
+    return;
+
+  blob->immutable = TRUE;
+}
+
+hb_bool_t
+hb_blob_is_immutable (hb_blob_t *blob)
+{
+  return blob->immutable;
+}
+
+
 unsigned int
 hb_blob_get_length (hb_blob_t *blob)
 {
@@ -205,62 +202,33 @@ hb_blob_get_length (hb_blob_t *blob)
 }
 
 const char *
-hb_blob_lock (hb_blob_t *blob)
+hb_blob_get_data (hb_blob_t *blob, unsigned int *length)
 {
-  if (hb_object_is_inert (blob))
+  if (length)
-    return NULL;
+    *length = blob->length;
-
-  hb_mutex_lock (&blob->lock);
-
-  (void) (HB_DEBUG_BLOB &&
-    fprintf (stderr, "%p %s (%d) -> %p\n", blob, HB_FUNC,
-	     blob->lock_count, blob->data));
-
-  blob->lock_count++;
-
-  hb_mutex_unlock (&blob->lock);
 
   return blob->data;
 }
 
-void
+char *
-hb_blob_unlock (hb_blob_t *blob)
+hb_blob_get_data_writable (hb_blob_t *blob, unsigned int *length)
 {
-  if (hb_object_is_inert (blob))
+  if (!_try_writable (blob)) {
-    return;
+    if (length)
+      *length = 0;
 
-  hb_mutex_lock (&blob->lock);
+    return NULL;
+  }
 
-  (void) (HB_DEBUG_BLOB &&
+  if (length)
-    fprintf (stderr, "%p %s (%d) -> %p\n", blob, HB_FUNC,
+    *length = blob->length;
-	     blob->lock_count, blob->data));
 
-  assert (blob->lock_count > 0);
+  return const_cast<char *> (blob->data);
-  blob->lock_count--;
-
-  hb_mutex_unlock (&blob->lock);
-}
-
-hb_bool_t
-hb_blob_is_writable (hb_blob_t *blob)
-{
-  hb_memory_mode_t mode;
-
-  if (hb_object_is_inert (blob))
-    return FALSE;
-
-  hb_mutex_lock (&blob->lock);
-
-  mode = blob->mode;
-
-  hb_mutex_unlock (&blob->lock);
-
-  return mode == HB_MEMORY_MODE_WRITABLE;
 }
 
 
 static hb_bool_t
-_try_make_writable_inplace_unix_locked (hb_blob_t *blob)
+_try_make_writable_inplace_unix (hb_blob_t *blob)
 {
 #if defined(HAVE_SYS_MMAN_H) && defined(HAVE_MPROTECT)
   uintptr_t pagesize = -1, mask, length;
@@ -295,6 +263,8 @@ _try_make_writable_inplace_unix_locked (hb_blob_t *blob)
     return FALSE;
   }
 
+  blob->mode = HB_MEMORY_MODE_WRITABLE;
+
   (void) (HB_DEBUG_BLOB &&
     fprintf (stderr, "%p %s: successfully made [%p..%p] (%lu bytes) writable\n",
 	     blob, HB_FUNC,
@@ -305,67 +275,59 @@ _try_make_writable_inplace_unix_locked (hb_blob_t *blob)
 #endif
 }
 
-static void
+static bool
-try_writable_inplace_locked (hb_blob_t *blob)
+_try_writable_inplace (hb_blob_t *blob)
 {
   (void) (HB_DEBUG_BLOB &&
-    fprintf (stderr, "%p %s: making writable\n", blob, HB_FUNC));
+    fprintf (stderr, "%p %s: making writable inplace\n", blob, HB_FUNC));
 
-  if (_try_make_writable_inplace_unix_locked (blob)) {
+  if (_try_make_writable_inplace_unix (blob))
-    (void) (HB_DEBUG_BLOB &&
+    return TRUE;
-      fprintf (stderr, "%p %s: making writable -> succeeded\n", blob, HB_FUNC));
+
-    blob->mode = HB_MEMORY_MODE_WRITABLE;
+  (void) (HB_DEBUG_BLOB &&
-  } else {
+    fprintf (stderr, "%p %s: making writable -> FAILED\n", blob, HB_FUNC));
-    (void) (HB_DEBUG_BLOB &&
+
-      fprintf (stderr, "%p %s: making writable -> FAILED\n", blob, HB_FUNC));
+  /* Failed to make writable inplace, mark that */
-    /* Failed to make writable inplace, mark that */
+  blob->mode = HB_MEMORY_MODE_READONLY;
-    blob->mode = HB_MEMORY_MODE_READONLY;
+  return FALSE;
-  }
 }
 
-hb_bool_t
+static bool
-hb_blob_try_writable (hb_blob_t *blob)
+_try_writable (hb_blob_t *blob)
 {
-  hb_memory_mode_t mode;
+  if (blob->immutable)
-
-  if (hb_object_is_inert (blob))
     return FALSE;
 
-  hb_mutex_lock (&blob->lock);
+  if (blob->mode == HB_MEMORY_MODE_WRITABLE)
+    return TRUE;
 
-  if (blob->mode == HB_MEMORY_MODE_READONLY_MAY_MAKE_WRITABLE)
+  if (blob->mode == HB_MEMORY_MODE_READONLY_MAY_MAKE_WRITABLE && _try_writable_inplace (blob))
-    try_writable_inplace_locked (blob);
+    return TRUE;
 
-  if (blob->mode == HB_MEMORY_MODE_READONLY)
+  if (blob->mode == HB_MEMORY_MODE_WRITABLE)
-  {
+    return TRUE;
-    char *new_data;
 
-    (void) (HB_DEBUG_BLOB &&
-      fprintf (stderr, "%p %s (%d) -> %p\n", blob, HB_FUNC,
-	       blob->lock_count, blob->data));
 
-    if (blob->lock_count)
+  (void) (HB_DEBUG_BLOB &&
-      goto done;
+    fprintf (stderr, "%p %s -> %p\n", blob, HB_FUNC, blob->data));
 
-    new_data = (char *) malloc (blob->length);
+  char *new_data;
-    if (new_data) {
-      (void) (HB_DEBUG_BLOB &&
-	fprintf (stderr, "%p %s: dupped successfully -> %p\n", blob, HB_FUNC, blob->data));
-      memcpy (new_data, blob->data, blob->length);
-      _hb_blob_destroy_user_data (blob);
-      blob->mode = HB_MEMORY_MODE_WRITABLE;
-      blob->data = new_data;
-      blob->user_data = new_data;
-      blob->destroy = free;
-    }
-  }
 
-done:
+  new_data = (char *) malloc (blob->length);
-  mode = blob->mode;
+  if (unlikely (!new_data))
+    return FALSE;
 
-  hb_mutex_unlock (&blob->lock);
+  (void) (HB_DEBUG_BLOB &&
+    fprintf (stderr, "%p %s: dupped successfully -> %p\n", blob, HB_FUNC, blob->data));
 
-  return mode == HB_MEMORY_MODE_WRITABLE;
+  memcpy (new_data, blob->data, blob->length);
+  _hb_blob_destroy_user_data (blob);
+  blob->mode = HB_MEMORY_MODE_WRITABLE;
+  blob->data = new_data;
+  blob->user_data = new_data;
+  blob->destroy = free;
+
+  return TRUE;
 }
 
 

src/hb-blob.h

@@ -74,20 +74,21 @@ hb_blob_get_user_data (hb_blob_t          *blob,
 		       hb_user_data_key_t *key);
 
 
+void
+hb_blob_make_immutable (hb_blob_t *blob);
+
+hb_bool_t
+hb_blob_is_immutable (hb_blob_t *blob);
+
+
 unsigned int
 hb_blob_get_length (hb_blob_t *blob);
 
 const char *
-hb_blob_lock (hb_blob_t *blob);
+hb_blob_get_data (hb_blob_t *blob, unsigned int *length);
 
-void
+char *
-hb_blob_unlock (hb_blob_t *blob);
+hb_blob_get_data_writable (hb_blob_t *blob, unsigned int *length);
-
-hb_bool_t
-hb_blob_is_writable (hb_blob_t *blob);
-
-hb_bool_t
-hb_blob_try_writable (hb_blob_t *blob);
 
 
 HB_END_DECLS

src/hb-font.cc

@@ -361,8 +361,6 @@ _hb_face_for_data_get_table (hb_tag_t tag, void *user_data)
 
   hb_blob_t *blob = hb_blob_create_sub_blob (data->blob, table.offset, table.length);
 
-  hb_blob_unlock (data->blob);
-
   return blob;
 }
 

src/hb-open-type-private.hh

@@ -186,9 +186,13 @@ struct hb_sanitize_context_t
   inline void init (hb_blob_t *blob)
   {
     this->blob = hb_blob_reference (blob);
-    this->start = hb_blob_lock (blob);
+    this->writable = false;
+  }
+
+  inline void setup (void)
+  {
+    this->start = hb_blob_get_data (blob, NULL);
     this->end = this->start + hb_blob_get_length (blob);
-    this->writable = hb_blob_is_writable (blob);
     this->edit_count = 0;
     this->debug_depth = 0;
 
@@ -204,7 +208,6 @@ struct hb_sanitize_context_t
       fprintf (stderr, "sanitize %p fini [%p..%p] %u edit requests\n",
 	       this->blob, this->start, this->end, this->edit_count));
 
-    hb_blob_unlock (this->blob);
     hb_blob_destroy (this->blob);
     this->blob = NULL;
     this->start = this->end = NULL;
@@ -286,11 +289,13 @@ struct Sanitizer
 
     /* TODO is_sane() stuff */
 
+    c->init (blob);
+
   retry:
     (void) (HB_DEBUG_SANITIZE &&
       fprintf (stderr, "Sanitizer %p start %s\n", blob, HB_FUNC));
 
-    c->init (blob);
+    c->setup ();
 
     if (unlikely (!c->start)) {
       c->finish ();
@@ -320,11 +325,17 @@ struct Sanitizer
     } else {
       unsigned int edit_count = c->edit_count;
       c->finish ();
-      if (edit_count && !hb_blob_is_writable (blob) && hb_blob_try_writable (blob)) {
+      if (edit_count && !c->writable) {
-        /* ok, we made it writable by relocating.  try again */
+        c->start = hb_blob_get_data_writable (blob, NULL);
-	(void) (HB_DEBUG_SANITIZE &&
+	c->end = c->start + hb_blob_get_length (blob);
-	  fprintf (stderr, "Sanitizer %p retry %s\n", blob, HB_FUNC));
+
-        goto retry;
+	if (c->start) {
+	  c->writable = true;
+	  /* ok, we made it writable by relocating.  try again */
+	  (void) (HB_DEBUG_SANITIZE &&
+	    fprintf (stderr, "Sanitizer %p retry %s\n", blob, HB_FUNC));
+	  goto retry;
+	}
       }
     }
 
@@ -339,7 +350,8 @@ struct Sanitizer
   }
 
   static const Type* lock_instance (hb_blob_t *blob) {
-    const char *base = hb_blob_lock (blob);
+    hb_blob_make_immutable (blob);
+    const char *base = hb_blob_get_data (blob, NULL);
     return unlikely (!base) ? &Null(Type) : CastP<Type> (base);
   }
 };

src/hb-ot-layout.cc

@@ -45,7 +45,7 @@ HB_BEGIN_DECLS
 hb_ot_layout_t *
 _hb_ot_layout_create (hb_face_t *face)
 {
-  /* Remove this object altogether */
+  /* TODO Remove this object altogether */
   hb_ot_layout_t *layout = (hb_ot_layout_t *) calloc (1, sizeof (hb_ot_layout_t));
 
   layout->gdef_blob = Sanitizer<GDEF>::sanitize (hb_face_reference_table (face, HB_OT_TAG_GDEF));
@@ -66,11 +66,6 @@ _hb_ot_layout_create (hb_face_t *face)
 void
 _hb_ot_layout_destroy (hb_ot_layout_t *layout)
 {
-  hb_blob_unlock (layout->gdef_blob);
-  hb_blob_unlock (layout->gsub_blob);
-  hb_blob_unlock (layout->gpos_blob);
-  hb_blob_unlock (layout->head_blob);
-
   hb_blob_destroy (layout->gdef_blob);
   hb_blob_destroy (layout->gsub_blob);
   hb_blob_destroy (layout->gpos_blob);

test/test-object.c

@@ -159,10 +159,10 @@ typedef struct {
   }
 static const object_t objects[] =
 {
-  OBJECT_WITHOUT_IMMUTABILITY (blob),
   OBJECT_WITHOUT_IMMUTABILITY (buffer),
   OBJECT_WITHOUT_IMMUTABILITY (face),
   OBJECT_WITHOUT_IMMUTABILITY (font),
+  OBJECT_WITH_IMMUTABILITY (blob),
   OBJECT_WITH_IMMUTABILITY (font_funcs),
   OBJECT_WITH_IMMUTABILITY (unicode_funcs)
 };