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).
diff --git a/objectivec/GPBMessage.m b/objectivec/GPBMessage.m index 7b1263f..20ae9ae 100644 --- a/objectivec/GPBMessage.m +++ b/objectivec/GPBMessage.m
@@ -71,6 +71,8 @@ @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 @@ 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 @@ // 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 @@ // 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 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); + 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; } - dispatch_semaphore_signal(self->readOnlySemaphore_); - return result; + + 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 index e132102..08dd358 100644 --- a/objectivec/GPBUtilities.m +++ b/objectivec/GPBUtilities.m
@@ -504,15 +504,6 @@ // 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 index a620288..3d3d734 100644 --- a/objectivec/GPBUtilities_PackagePrivate.h +++ b/objectivec/GPBUtilities_PackagePrivate.h
@@ -289,10 +289,6 @@ 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,