Aggregate Metadata Files (protocolbuffers#7155) (protocolbuffers#7194)

Instead of calling initOnce of dependencies, initialize metadata of dependencies in the same file. Needs to pass aggregate_metadata option to protoc to trigger, e.g.: --php_out=aggregate_metadata=foo#bar:generated_dir For each input file, transitive dependencies (including itself), whose package name has the prefix of foo or bar, will be aggregated, in which their metadata string will be aggregated in the same internalAddGeneratedFile call. For other dependencies, initOnce is called as before. This feature is EXPERIMENTAL. DO NOT USE!!!
JoseRFJuniorLLMs · Feb 11, 2020 · 537c5aa · 537c5aa
1 parent fedb2be
commit 537c5aa
Show file tree

Hide file tree

Showing 8 changed files with 387 additions and 145 deletions.
diff --git a/php/composer.json b/php/composer.json
@@ -23,6 +23,7 @@
     }
   },
   "scripts": {
-    "test": "(cd tests && rm -rf generated && mkdir -p generated && ../../src/protoc --php_out=generated -I../../src -I. proto/empty/echo.proto proto/test.proto proto/test_include.proto proto/test_no_namespace.proto proto/test_prefix.proto proto/test_php_namespace.proto proto/test_empty_php_namespace.proto proto/test_reserved_enum_lower.proto proto/test_reserved_enum_upper.proto proto/test_reserved_enum_value_lower.proto proto/test_reserved_enum_value_upper.proto proto/test_reserved_message_lower.proto proto/test_reserved_message_upper.proto proto/test_service.proto proto/test_service_namespace.proto proto/test_wrapper_type_setters.proto proto/test_descriptors.proto) && (cd ../src && ./protoc --php_out=../php/tests/generated -I../php/tests -I. ../php/tests/proto/test_import_descriptor_proto.proto) && vendor/bin/phpunit"
+    "test": "(cd tests && rm -rf generated && mkdir -p generated && ../../src/protoc --php_out=generated -I../../src -I. proto/empty/echo.proto proto/test.proto proto/test_include.proto proto/test_no_namespace.proto proto/test_prefix.proto proto/test_php_namespace.proto proto/test_empty_php_namespace.proto proto/test_reserved_enum_lower.proto proto/test_reserved_enum_upper.proto proto/test_reserved_enum_value_lower.proto proto/test_reserved_enum_value_upper.proto proto/test_reserved_message_lower.proto proto/test_reserved_message_upper.proto proto/test_service.proto proto/test_service_namespace.proto proto/test_wrapper_type_setters.proto proto/test_descriptors.proto) && (cd ../src && ./protoc --php_out=../php/tests/generated -I../php/tests -I. ../php/tests/proto/test_import_descriptor_proto.proto) && vendor/bin/phpunit",
+    "aggregate_metadata_test": "(cd tests && rm -rf generated && mkdir -p generated && ../../src/protoc --php_out=aggregate_metadata=foo#bar:generated -I../../src -I. proto/test.proto proto/test_include.proto && ../../src/protoc --php_out=generated -I../../src -I. proto/empty/echo.proto proto/test_no_namespace.proto proto/test_empty_php_namespace.proto proto/test_prefix.proto proto/test_php_namespace.proto proto/test_reserved_enum_lower.proto proto/test_reserved_enum_upper.proto proto/test_reserved_enum_value_lower.proto proto/test_reserved_enum_value_upper.proto proto/test_reserved_message_lower.proto proto/test_reserved_message_upper.proto proto/test_service.proto proto/test_service_namespace.proto proto/test_wrapper_type_setters.proto proto/test_descriptors.proto) && (cd ../src && ./protoc --php_out=aggregate_metadata=foo:../php/tests/generated -I../php/tests -I. ../php/tests/proto/test_import_descriptor_proto.proto) && vendor/bin/phpunit"
   }
 }
diff --git a/php/ext/google/protobuf/def.c b/php/ext/google/protobuf/def.c
@@ -891,74 +891,15 @@ bool depends_on_descriptor(const google_protobuf_FileDescriptorProto* file) {
   return false;
 }
 
-const upb_filedef *parse_and_add_descriptor(const char *data,
-                                            PHP_PROTO_SIZE data_len,
-                                            InternalDescriptorPoolImpl *pool,
-                                            upb_arena *arena) {
-  size_t n;
-  google_protobuf_FileDescriptorSet *set;
-  const google_protobuf_FileDescriptorProto* const* files;
-  const upb_filedef* file;
-  upb_status status;
-
-  set = google_protobuf_FileDescriptorSet_parse(
-      data, data_len, arena);
-
-  if (!set) {
-    zend_error(E_ERROR, "Failed to parse binary descriptor\n");
-    return NULL;
-  }
-
-  files = google_protobuf_FileDescriptorSet_file(set, &n);
-
-  if (n != 1) {
-    zend_error(E_ERROR, "Serialized descriptors should have exactly one file");
-    return NULL;
-  }
-
-  // Check whether file has already been added.
-  upb_strview name = google_protobuf_FileDescriptorProto_name(files[0]);
-  // TODO(teboring): Needs another look up method which takes data and length.
-  file = upb_symtab_lookupfile2(pool->symtab, name.data, name.size);
-  if (file != NULL) {
-    return NULL;
-  }
-
-  // The PHP code generator currently special-cases descriptor.proto.  It
-  // doesn't add it as a dependency even if the proto file actually does
-  // depend on it.
-  if (depends_on_descriptor(files[0]) &&
-      upb_symtab_lookupfile(pool->symtab, "google/protobuf/descriptor.proto") ==
-          NULL) {
-    if (!parse_and_add_descriptor((char *)descriptor_proto,
-                                  descriptor_proto_len, pool, arena)) {
-      return NULL;
-    }
-  }
-
-  upb_status_clear(&status);
-  file = upb_symtab_addfile(pool->symtab, files[0], &status);
-  check_upb_status(&status, "Unable to load descriptor");
-  return file;
-}
-
-void internal_add_generated_file(const char *data, PHP_PROTO_SIZE data_len,
-                                 InternalDescriptorPoolImpl *pool,
-                                 bool use_nested_submsg TSRMLS_DC) {
-  int i;
-  upb_arena *arena;
-  const upb_filedef* file;
-
-  arena = upb_arena_new();
-  file = parse_and_add_descriptor(data, data_len, pool, arena);
-  upb_arena_free(arena);
-  if (!file) return;
-
+static void internal_add_single_generated_file(
+    const upb_filedef* file,
+    InternalDescriptorPoolImpl* pool,
+    bool use_nested_submsg TSRMLS_DC) {
+  size_t i;
   // For each enum/message, we need its PHP class, upb descriptor and its PHP
   // wrapper. These information are needed later for encoding, decoding and type
   // checking. However, sometimes we just have one of them. In order to find
   // them quickly, here, we store the mapping for them.
-
   for (i = 0; i < upb_filedef_msgcount(file); i++) {
     const upb_msgdef *msgdef = upb_filedef_msg(file, i);
     CREATE_HASHTABLE_VALUE(desc, desc_php, Descriptor, descriptor_type);
@@ -1000,6 +941,73 @@ void internal_add_generated_file(const char *data, PHP_PROTO_SIZE data_len,
   }
 }
 
+const bool parse_and_add_descriptor(const char *data,
+                                    PHP_PROTO_SIZE data_len,
+                                    InternalDescriptorPoolImpl *pool,
+                                    upb_arena *arena,
+                                    bool use_nested_submsg TSRMLS_DC) {
+  size_t i, n;
+  google_protobuf_FileDescriptorSet *set;
+  const google_protobuf_FileDescriptorProto* const* files;
+  const upb_filedef* file;
+  upb_status status;
+
+  set = google_protobuf_FileDescriptorSet_parse(
+      data, data_len, arena);
+
+  if (!set) {
+    zend_error(E_ERROR, "Failed to parse binary descriptor\n");
+    return false;
+  }
+
+  files = google_protobuf_FileDescriptorSet_file(set, &n);
+
+  for (i = 0; i < n; i++) {
+    // Check whether file has already been added.
+    upb_strview name = google_protobuf_FileDescriptorProto_name(files[i]);
+    // TODO(teboring): Needs another look up method which takes data and length.
+    file = upb_symtab_lookupfile2(pool->symtab, name.data, name.size);
+    if (file != NULL) {
+      continue;
+    }
+
+    // The PHP code generator currently special-cases descriptor.proto.  It
+    // doesn't add it as a dependency even if the proto file actually does
+    // depend on it.
+    if (depends_on_descriptor(files[i]) &&
+        upb_symtab_lookupfile(
+            pool->symtab, "google/protobuf/descriptor.proto") ==
+                NULL) {
+      if (!parse_and_add_descriptor((char *)descriptor_proto,
+                                    descriptor_proto_len, pool, arena,
+                                    use_nested_submsg TSRMLS_CC)) {
+        return false;
+      }
+    }
+
+    upb_status_clear(&status);
+    file = upb_symtab_addfile(pool->symtab, files[i], &status);
+    check_upb_status(&status, "Unable to load descriptor");
+
+    internal_add_single_generated_file(file, pool, use_nested_submsg TSRMLS_CC);
+  }
+
+  return true;
+}
+
+void internal_add_generated_file(const char *data, PHP_PROTO_SIZE data_len,
+                                 InternalDescriptorPoolImpl *pool,
+                                 bool use_nested_submsg TSRMLS_DC) {
+  int i;
+  upb_arena *arena;
+
+  arena = upb_arena_new();
+  parse_and_add_descriptor(data, data_len, pool, arena,
+                           use_nested_submsg TSRMLS_CC);
+  upb_arena_free(arena);
+  return;
+}
+
 PHP_METHOD(InternalDescriptorPool, internalAddGeneratedFile) {
   char *data = NULL;
   PHP_PROTO_SIZE data_len;

diff --git a/php/src/Google/Protobuf/Internal/DescriptorPool.php b/php/src/Google/Protobuf/Internal/DescriptorPool.php
@@ -59,22 +59,25 @@ public function internalAddGeneratedFile($data, $use_nested = false)
     {
         $files = new FileDescriptorSet();
         $files->mergeFromString($data);
-        $file = FileDescriptor::buildFromProto($files->getFile()[0]);
 
-        foreach ($file->getMessageType() as $desc) {
-            $this->addDescriptor($desc);
-        }
-        unset($desc);
+        foreach($files->getFile() as $file_proto) {
+            $file = FileDescriptor::buildFromProto($file_proto);
 
-        foreach ($file->getEnumType() as $desc) {
-            $this->addEnumDescriptor($desc);
-        }
-        unset($desc);
+            foreach ($file->getMessageType() as $desc) {
+                $this->addDescriptor($desc);
+            }
+            unset($desc);
 
-        foreach ($file->getMessageType() as $desc) {
-            $this->crossLink($desc);
+            foreach ($file->getEnumType() as $desc) {
+                $this->addEnumDescriptor($desc);
+            }
+            unset($desc);
+
+            foreach ($file->getMessageType() as $desc) {
+                $this->crossLink($desc);
+            }
+            unset($desc);
         }
-        unset($desc);
     }
 
     public function addMessage($name, $klass)
@@ -149,8 +152,13 @@ private function crossLink(Descriptor $desc)
             switch ($field->getType()) {
                 case GPBType::MESSAGE:
                     $proto = $field->getMessageType();
-                    $field->setMessageType(
-                        $this->getDescriptorByProtoName($proto));
+                    $subdesc = $this->getDescriptorByProtoName($proto);
+                    if (is_null($subdesc)) {
+                        trigger_error(
+                            'proto not added: ' . $proto
+                            . " for " . $desc->getFullName(), E_ERROR);
+                    }
+                    $field->setMessageType($subdesc);
                     break;
                 case GPBType::ENUM:
                     $proto = $field->getEnumType();

diff --git a/php/src/Google/Protobuf/Internal/Message.php b/php/src/Google/Protobuf/Internal/Message.php
@@ -94,6 +94,7 @@ private function initWithGeneratedPool()
         $this->desc = $pool->getDescriptorByClassName(get_class($this));
         if (is_null($this->desc)) {
             user_error(get_class($this) . " is not found in descriptor pool.");
+            return;
         }
         foreach ($this->desc->getField() as $field) {
             $setter = $field->getSetter();

diff --git a/php/tests/proto/test_import_descriptor_proto.proto b/php/tests/proto/test_import_descriptor_proto.proto
@@ -1,5 +1,7 @@
 syntax = "proto3";
 
+package foo;
+
 import "google/protobuf/descriptor.proto";
 
 message TestImportDescriptorProto {

diff --git a/php/tests/well_known_test.php b/php/tests/well_known_test.php
@@ -4,6 +4,7 @@
 require_once('test_util.php');
 
 use Foo\TestMessage;
+use Foo\TestImportDescriptorProto;
 use Google\Protobuf\Any;
 use Google\Protobuf\Api;
 use Google\Protobuf\BoolValue;