From 39996cfa128983f540923ba93ce02c9420596542 Mon Sep 17 00:00:00 2001 From: theodorerose Date: Wed, 22 Jun 2022 22:17:58 +0000 Subject: [PATCH 1/2] cherry-pick: arenastring --- src/google/protobuf/arena_impl.h | 21 ++++++++++++++------- src/google/protobuf/arenastring.cc | 6 ++---- src/google/protobuf/arenastring.h | 27 +++++++++++++++++---------- 3 files changed, 33 insertions(+), 21 deletions(-) diff --git a/src/google/protobuf/arena_impl.h b/src/google/protobuf/arena_impl.h index d02ad93985ed..76727688b558 100644 --- a/src/google/protobuf/arena_impl.h +++ b/src/google/protobuf/arena_impl.h @@ -55,6 +55,13 @@ namespace google { namespace protobuf { namespace internal { +// To prevent sharing cache lines between threads +#ifdef __cpp_aligned_new +enum { kCacheAlignment = 64 }; +#else +enum { kCacheAlignment = alignof(max_align_t) }; // do the best we can +#endif + inline constexpr size_t AlignUpTo8(size_t n) { // Align n to next multiple of 8 (from Hacker's Delight, Chapter 3.) return (n + 7) & static_cast(-8); @@ -497,10 +504,10 @@ class PROTOBUF_EXPORT ThreadSafeArena { // have fallback function calls in tail position. This substantially improves // code for the happy path. PROTOBUF_NDEBUG_INLINE bool MaybeAllocateAligned(size_t n, void** out) { - SerialArena* a; + SerialArena* arena; if (PROTOBUF_PREDICT_TRUE(!alloc_policy_.should_record_allocs() && - GetSerialArenaFromThreadCache(&a))) { - return a->MaybeAllocateAligned(n, out); + GetSerialArenaFromThreadCache(&arena))) { + return arena->MaybeAllocateAligned(n, out); } return false; } @@ -564,7 +571,7 @@ class PROTOBUF_EXPORT ThreadSafeArena { // fast path optimizes the case where a single thread uses multiple arenas. ThreadCache* tc = &thread_cache(); SerialArena* serial = hint_.load(std::memory_order_acquire); - if (PROTOBUF_PREDICT_TRUE(serial != NULL && serial->owner() == tc)) { + if (PROTOBUF_PREDICT_TRUE(serial != nullptr && serial->owner() == tc)) { *arena = serial; return true; } @@ -602,7 +609,7 @@ class PROTOBUF_EXPORT ThreadSafeArena { #ifdef _MSC_VER #pragma warning(disable : 4324) #endif - struct alignas(64) ThreadCache { + struct alignas(kCacheAlignment) ThreadCache { #if defined(GOOGLE_PROTOBUF_NO_THREADLOCAL) // If we are using the ThreadLocalStorage class to store the ThreadCache, // then the ThreadCache's default constructor has to be responsible for @@ -610,7 +617,7 @@ class PROTOBUF_EXPORT ThreadSafeArena { ThreadCache() : next_lifecycle_id(0), last_lifecycle_id_seen(-1), - last_serial_arena(NULL) {} + last_serial_arena(nullptr) {} #endif // Number of per-thread lifecycle IDs to reserve. Must be power of two. @@ -633,7 +640,7 @@ class PROTOBUF_EXPORT ThreadSafeArena { #ifdef _MSC_VER #pragma warning(disable : 4324) #endif - struct alignas(64) CacheAlignedLifecycleIdGenerator { + struct alignas(kCacheAlignment) CacheAlignedLifecycleIdGenerator { std::atomic id; }; static CacheAlignedLifecycleIdGenerator lifecycle_id_generator_; diff --git a/src/google/protobuf/arenastring.cc b/src/google/protobuf/arenastring.cc index d886ddad66a5..3887c940a3e1 100644 --- a/src/google/protobuf/arenastring.cc +++ b/src/google/protobuf/arenastring.cc @@ -187,7 +187,7 @@ std::string* ArenaStringPtr::Release() { if (IsDefault()) return nullptr; std::string* released = tagged_ptr_.Get(); - if (!tagged_ptr_.IsAllocated()) { + if (tagged_ptr_.IsArena()) { released = tagged_ptr_.IsMutable() ? new std::string(std::move(*released)) : new std::string(*released); } @@ -216,9 +216,7 @@ void ArenaStringPtr::SetAllocated(std::string* value, Arena* arena) { } void ArenaStringPtr::Destroy() { - if (tagged_ptr_.IsAllocated()) { - delete tagged_ptr_.Get(); - } + delete tagged_ptr_.GetIfAllocated(); } void ArenaStringPtr::ClearToEmpty() { diff --git a/src/google/protobuf/arenastring.h b/src/google/protobuf/arenastring.h index b8d31fec015b..6bc8395f233f 100644 --- a/src/google/protobuf/arenastring.h +++ b/src/google/protobuf/arenastring.h @@ -96,13 +96,12 @@ class PROTOBUF_EXPORT LazyString { class TaggedStringPtr { public: - // Bit flags qualifying string properties. We can use up to 3 bits as - // ptr_ is guaranteed and enforced to be aligned on 8 byte boundaries. + // Bit flags qualifying string properties. We can use 2 bits as + // ptr_ is guaranteed and enforced to be aligned on 4 byte boundaries. enum Flags { kArenaBit = 0x1, // ptr is arena allocated - kAllocatedBit = 0x2, // ptr is heap allocated - kMutableBit = 0x4, // ptr contents are fully mutable - kMask = 0x7 // Bit mask + kMutableBit = 0x2, // ptr contents are fully mutable + kMask = 0x3 // Bit mask }; // Composed logical types @@ -112,7 +111,7 @@ class TaggedStringPtr { // Allocated strings are mutable and (as the name implies) owned. // A heap allocated string must be deleted. - kAllocated = kAllocatedBit | kMutableBit, + kAllocated = kMutableBit, // Mutable arena strings are strings where the string instance is owned // by the arena, but the string contents itself are owned by the string @@ -166,8 +165,16 @@ class TaggedStringPtr { // Returns true if the current string is an immutable default value. inline bool IsDefault() const { return (as_int() & kMask) == kDefault; } - // Returns true if the current string is a heap allocated mutable value. - inline bool IsAllocated() const { return as_int() & kAllocatedBit; } + // If the current string is a heap-allocated mutable value, returns a pointer + // to it. Returns nullptr otherwise. + inline std::string *GetIfAllocated() const { + auto allocated = as_int() ^ kAllocated; + if (allocated & kMask) return nullptr; + + auto ptr = reinterpret_cast(allocated); + PROTOBUF_ASSUME(ptr != nullptr); + return ptr; + } // Returns true if the current string is an arena allocated value. // This means it's either a mutable or fixed size arena string. @@ -224,8 +231,8 @@ static_assert(std::is_trivial::value, // Because ArenaStringPtr is used in oneof unions, its constructor is a NOP and // the field is always manually initialized via method calls. // -// See TaggedPtr for more information about the types of string values being -// held, and the mutable and ownership invariants for each type. +// See TaggedStringPtr for more information about the types of string values +// being held, and the mutable and ownership invariants for each type. struct PROTOBUF_EXPORT ArenaStringPtr { ArenaStringPtr() = default; constexpr ArenaStringPtr(ExplicitlyConstructedArenaString* default_value, From 71adb837e7aa4c2405b6a9481257198d9b4217b6 Mon Sep 17 00:00:00 2001 From: theodorerose Date: Wed, 22 Jun 2022 22:26:53 +0000 Subject: [PATCH 2/2] cherry-pick c38281dd20e562bac239bc77ab2fa10f71661708 --- php/src/Google/Protobuf/FieldDescriptor.php | 35 +++++++++++++---- .../Protobuf/Internal/FieldDescriptor.php | 38 +++++++++++++++++-- .../Protobuf/Internal/OneofDescriptor.php | 9 +++++ php/src/Google/Protobuf/OneofDescriptor.php | 9 ++++- 4 files changed, 79 insertions(+), 12 deletions(-) diff --git a/php/src/Google/Protobuf/FieldDescriptor.php b/php/src/Google/Protobuf/FieldDescriptor.php index 6d08cea9da97..ac919a24a97b 100644 --- a/php/src/Google/Protobuf/FieldDescriptor.php +++ b/php/src/Google/Protobuf/FieldDescriptor.php @@ -39,6 +39,7 @@ class FieldDescriptor { use GetPublicDescriptorTrait; + /** @var \Google\Protobuf\Internal\FieldDescriptor $internal_desc */ private $internal_desc; /** @@ -81,6 +82,32 @@ public function getType() return $this->internal_desc->getType(); } + /** + * @return OneofDescriptor + */ + public function getContainingOneof() + { + return $this->getPublicDescriptor($this->internal_desc->getContainingOneof()); + } + + /** + * Gets the field's containing oneof, only if non-synthetic. + * + * @return null|OneofDescriptor + */ + public function getRealContainingOneof() + { + return $this->getPublicDescriptor($this->internal_desc->getRealContainingOneof()); + } + + /** + * @return boolean + */ + public function hasOptionalKeyword() + { + return $this->internal_desc->hasOptionalKeyword(); + } + /** * @return Descriptor Returns a descriptor for the field type if the field type is a message, otherwise throws \Exception * @throws \Exception @@ -114,12 +141,4 @@ public function isMap() { return $this->internal_desc->isMap(); } - - /** - * @return boolean - */ - public function hasOptionalKeyword() - { - return $this->internal_desc->hasOptionalKeyword(); - } } diff --git a/php/src/Google/Protobuf/Internal/FieldDescriptor.php b/php/src/Google/Protobuf/Internal/FieldDescriptor.php index ce83f63a2b26..3a9a73b729ff 100644 --- a/php/src/Google/Protobuf/Internal/FieldDescriptor.php +++ b/php/src/Google/Protobuf/Internal/FieldDescriptor.php @@ -46,8 +46,11 @@ class FieldDescriptor private $message_type; private $enum_type; private $packed; - private $is_map; private $oneof_index = -1; + private $proto3_optional; + + /** @var OneofDescriptor $containing_oneof */ + private $containing_oneof; public function __construct() { @@ -169,6 +172,32 @@ public function getPacked() return $this->packed; } + public function getProto3Optional() + { + return $this->proto3_optional; + } + + public function setProto3Optional($proto3_optional) + { + $this->proto3_optional = $proto3_optional; + } + + public function getContainingOneof() + { + return $this->containing_oneof; + } + + public function setContainingOneof($containing_oneof) + { + $this->containing_oneof = $containing_oneof; + } + + public function getRealContainingOneof() + { + return !is_null($this->containing_oneof) && !$this->containing_oneof->isSynthetic() + ? $this->containing_oneof : null; + } + public function isPackable() { return $this->isRepeated() && self::isTypePackable($this->type); @@ -214,6 +243,10 @@ private static function isTypePackable($field_type) $field_type !== GPBType::BYTES); } + /** + * @param FieldDescriptorProto $proto + * @return FieldDescriptor + */ public static function getFieldDescriptor($proto) { $type_name = null; @@ -248,8 +281,6 @@ public static function getFieldDescriptor($proto) $field = new FieldDescriptor(); $field->setName($proto->getName()); - $json_name = $proto->hasJsonName() ? $proto->getJsonName() : - lcfirst(implode('', array_map('ucwords', explode('_', $proto->getName())))); if ($proto->hasJsonName()) { $json_name = $proto->getJsonName(); } else { @@ -269,6 +300,7 @@ public static function getFieldDescriptor($proto) $field->setLabel($proto->getLabel()); $field->setPacked($packed); $field->setOneofIndex($oneof_index); + $field->setProto3Optional($proto->getProto3Optional()); // At this time, the message/enum type may have not been added to pool. // So we use the type name as place holder and will replace it with the diff --git a/php/src/Google/Protobuf/Internal/OneofDescriptor.php b/php/src/Google/Protobuf/Internal/OneofDescriptor.php index 67b107f6a4b6..432368571023 100644 --- a/php/src/Google/Protobuf/Internal/OneofDescriptor.php +++ b/php/src/Google/Protobuf/Internal/OneofDescriptor.php @@ -37,6 +37,7 @@ class OneofDescriptor use HasPublicDescriptorTrait; private $name; + /** @var \Google\Protobuf\FieldDescriptor[] $fields */ private $fields; public function __construct() @@ -64,13 +65,21 @@ public function getFields() return $this->fields; } + public function isSynthetic() + { + return !is_null($this->fields) && count($this->fields) === 1 + && $this->fields[0]->getProto3Optional(); + } + public static function buildFromProto($oneof_proto, $desc, $index) { $oneof = new OneofDescriptor(); $oneof->setName($oneof_proto->getName()); foreach ($desc->getField() as $field) { + /** @var FieldDescriptor $field */ if ($field->getOneofIndex() == $index) { $oneof->addField($field); + $field->setContainingOneof($oneof); } } return $oneof; diff --git a/php/src/Google/Protobuf/OneofDescriptor.php b/php/src/Google/Protobuf/OneofDescriptor.php index 92b4e279dac3..66ffbd5caf9d 100644 --- a/php/src/Google/Protobuf/OneofDescriptor.php +++ b/php/src/Google/Protobuf/OneofDescriptor.php @@ -38,6 +38,7 @@ class OneofDescriptor { use GetPublicDescriptorTrait; + /** @var \Google\Protobuf\Internal\OneofDescriptor $internal_desc */ private $internal_desc; /** @@ -62,6 +63,12 @@ public function getName() */ public function getField($index) { + if ( + is_null($this->internal_desc->getFields()) + || !isset($this->internal_desc->getFields()[$index]) + ) { + return null; + } return $this->getPublicDescriptor($this->internal_desc->getFields()[$index]); } @@ -75,6 +82,6 @@ public function getFieldCount() public function isSynthetic() { - return $this->internal_desc->isSynthetic(); + return $this->internal_desc->isSynthetic(); } }