Switch to atomic for setting autocreated objects.

- Update semaphore comment to new scope. - Use an atomic swap to avoid needing to use the semaphore. This means the semaphore is create only when extension are auto created (less memory usage).
protocolbuffers · Oct 26, 2020 · 2123ed5 · 2123ed5
1 parent de5d1b9
commit 2123ed5
Show file tree

Hide file tree

Showing 3 changed files with 77 additions and 61 deletions.
diff --git a/objectivec/GPBMessage.m b/objectivec/GPBMessage.m
@@ -71,6 +71,8 @@ @interface GPBMessage () {
  @package
   GPBUnknownFieldSet *unknownFields_;
   NSMutableDictionary *extensionMap_;
+  // Readonly access to autocreatedExtensionMap_ is protected via
+  // readOnlySemaphore_.
   NSMutableDictionary *autocreatedExtensionMap_;
 
   // If the object was autocreated, we remember the creator so that if we get
@@ -79,10 +81,10 @@ @interface GPBMessage () {
   GPBFieldDescriptor *autocreatorField_;
   GPBExtensionDescriptor *autocreatorExtension_;
 
-  // A lock to provide mutual exclusion from internal data that can be modified
-  // by *read* operations such as getters (autocreation of message fields and
-  // message extensions, not setting of values). Used to guarantee thread safety
-  // for concurrent reads on the message.
+  // Message can only be mutated from one thread. But some *readonly* operations
+  // modifify internal state because they autocreate things. The
+  // autocreatedExtensionMap_ is one such structure. Access during readonly
+  // operations is protected via this semaphore.
   // NOTE: OSSpinLock may seem like a good fit here but Apple engineers have
   // pointed out that they are vulnerable to live locking on iOS in cases of
   // priority inversion:
@@ -583,19 +585,30 @@ static id GetOrCreateArrayIvarWithField(GPBMessage *self,
 // This is like GPBGetObjectIvarWithField(), but for arrays, it should
 // only be used to wire the method into the class.
 static id GetArrayIvarWithField(GPBMessage *self, GPBFieldDescriptor *field) {
-  id array = GPBGetObjectIvarWithFieldNoAutocreate(self, field);
-  if (!array) {
-    // Check again after getting the lock.
-    GPBPrepareReadOnlySemaphore(self);
-    dispatch_semaphore_wait(self->readOnlySemaphore_, DISPATCH_TIME_FOREVER);
-    array = GPBGetObjectIvarWithFieldNoAutocreate(self, field);
-    if (!array) {
-      array = CreateArrayForField(field, self);
-      GPBSetAutocreatedRetainedObjectIvarWithField(self, field, array);
-    }
-    dispatch_semaphore_signal(self->readOnlySemaphore_);
+  uint8_t *storage = (uint8_t *)self->messageStorage_;
+  _Atomic(id) *typePtr = (_Atomic(id) *)&storage[field->description_->offset];
+  id array = atomic_load(typePtr);
+  if (array) {
+    return array;
   }
-  return array;
+
+  id expected = nil;
+  id autocreated = CreateArrayForField(field, self);
+  if (atomic_compare_exchange_strong(typePtr, &expected, autocreated)) {
+    // Value was set, return it.
+    return autocreated;
+  }
+
+  // Some other thread set it, release the one created and return what got set.
+  if (GPBFieldDataTypeIsObject(field)) {
+    GPBAutocreatedArray *autoArray = autocreated;
+    autoArray->_autocreator = nil;
+  } else {
+    GPBInt32Array *gpbArray = autocreated;
+    gpbArray->_autocreator = nil;
+  }
+  [autocreated release];
+  return expected;
 }
 
 static id GetOrCreateMapIvarWithField(GPBMessage *self,
@@ -613,19 +626,31 @@ static id GetOrCreateMapIvarWithField(GPBMessage *self,
 // This is like GPBGetObjectIvarWithField(), but for maps, it should
 // only be used to wire the method into the class.
 static id GetMapIvarWithField(GPBMessage *self, GPBFieldDescriptor *field) {
-  id dict = GPBGetObjectIvarWithFieldNoAutocreate(self, field);
-  if (!dict) {
-    // Check again after getting the lock.
-    GPBPrepareReadOnlySemaphore(self);
-    dispatch_semaphore_wait(self->readOnlySemaphore_, DISPATCH_TIME_FOREVER);
-    dict = GPBGetObjectIvarWithFieldNoAutocreate(self, field);
-    if (!dict) {
-      dict = CreateMapForField(field, self);
-      GPBSetAutocreatedRetainedObjectIvarWithField(self, field, dict);
-    }
-    dispatch_semaphore_signal(self->readOnlySemaphore_);
+  uint8_t *storage = (uint8_t *)self->messageStorage_;
+  _Atomic(id) *typePtr = (_Atomic(id) *)&storage[field->description_->offset];
+  id dict = atomic_load(typePtr);
+  if (dict) {
+    return dict;
   }
-  return dict;
+
+  id expected = nil;
+  id autocreated = CreateMapForField(field, self);
+  if (atomic_compare_exchange_strong(typePtr, &expected, autocreated)) {
+    // Value was set, return it.
+    return autocreated;
+  }
+
+  // Some other thread set it, release the one created and return what got set.
+  if ((field.mapKeyDataType == GPBDataTypeString) &&
+      GPBFieldDataTypeIsObject(field)) {
+    GPBAutocreatedDictionary *autoDict = autocreated;
+    autoDict->_autocreator = nil;
+  } else {
+    GPBInt32Int32Dictionary *gpbDict = autocreated;
+    gpbDict->_autocreator = nil;
+  }
+  [autocreated release];
+  return expected;
 }
 
 #endif  // !defined(__clang_analyzer__)
@@ -3337,30 +3362,34 @@ id GPBGetMessageMapField(GPBMessage *self, GPBFieldDescriptor *field) {
 
 id GPBGetObjectIvarWithField(GPBMessage *self, GPBFieldDescriptor *field) {
   NSCAssert(!GPBFieldIsMapOrArray(field), @"Shouldn't get here");
-  if (GPBGetHasIvarField(self, field)) {
-    uint8_t *storage = (uint8_t *)self->messageStorage_;
-    id *typePtr = (id *)&storage[field->description_->offset];
-    return *typePtr;
-  }
-  // Not set...
-
-  // Non messages (string/data), get their default.
   if (!GPBFieldDataTypeIsMessage(field)) {
+    if (GPBGetHasIvarField(self, field)) {
+      uint8_t *storage = (uint8_t *)self->messageStorage_;
+      id *typePtr = (id *)&storage[field->description_->offset];
+      return *typePtr;
+    }
+    // Not set...non messages (string/data), get their default.
     return field.defaultValue.valueMessage;
   }
 
-  GPBPrepareReadOnlySemaphore(self);
-  dispatch_semaphore_wait(self->readOnlySemaphore_, DISPATCH_TIME_FOREVER);
-  GPBMessage *result = GPBGetObjectIvarWithFieldNoAutocreate(self, field);
-  if (!result) {
-    // For non repeated messages, create the object, set it and return it.
-    // This object will not initially be visible via GPBGetHasIvar, so
-    // we save its creator so it can become visible if it's mutated later.
-    result = GPBCreateMessageWithAutocreator(field.msgClass, self, field);
-    GPBSetAutocreatedRetainedObjectIvarWithField(self, field, result);
-  }
-  dispatch_semaphore_signal(self->readOnlySemaphore_);
-  return result;
+  uint8_t *storage = (uint8_t *)self->messageStorage_;
+  _Atomic(id) *typePtr = (_Atomic(id) *)&storage[field->description_->offset];
+  id msg = atomic_load(typePtr);
+  if (msg) {
+    return msg;
+  }
+
+  id expected = nil;
+  id autocreated = GPBCreateMessageWithAutocreator(field.msgClass, self, field);
+  if (atomic_compare_exchange_strong(typePtr, &expected, autocreated)) {
+    // Value was set, return it.
+    return autocreated;
+  }
+
+  // Some other thread set it, release the one created and return what got set.
+  GPBClearMessageAutocreator(autocreated);
+  [autocreated release];
+  return expected;
 }
 
 #pragma clang diagnostic pop
diff --git a/objectivec/GPBUtilities.m b/objectivec/GPBUtilities.m
@@ -504,15 +504,6 @@ static void GPBMaybeClearOneofPrivate(GPBMessage *self,
 // Object types are handled slightly differently, they need to be released
 // and retained.
 
-void GPBSetAutocreatedRetainedObjectIvarWithField(
-    GPBMessage *self, GPBFieldDescriptor *field,
-    id __attribute__((ns_consumed)) value) {
-  uint8_t *storage = (uint8_t *)self->messageStorage_;
-  id *typePtr = (id *)&storage[field->description_->offset];
-  NSCAssert(*typePtr == NULL, @"Can't set autocreated object more than once.");
-  *typePtr = value;
-}
-
 void GPBClearAutocreatedMessageIvarWithField(GPBMessage *self,
                                              GPBFieldDescriptor *field) {
   if (GPBGetHasIvarField(self, field)) {

diff --git a/objectivec/GPBUtilities_PackagePrivate.h b/objectivec/GPBUtilities_PackagePrivate.h
@@ -289,10 +289,6 @@ void GPBSetRetainedObjectIvarWithFieldPrivate(GPBMessage *self,
 id GPBGetObjectIvarWithFieldNoAutocreate(GPBMessage *self,
                                          GPBFieldDescriptor *field);
 
-void GPBSetAutocreatedRetainedObjectIvarWithField(
-    GPBMessage *self, GPBFieldDescriptor *field,
-    id __attribute__((ns_consumed)) value);
-
 // Clears and releases the autocreated message ivar, if it's autocreated. If
 // it's not set as autocreated, this method does nothing.
 void GPBClearAutocreatedMessageIvarWithField(GPBMessage *self,