p4_pdpi: avoid copies on the PacketIn/PacketOut translation hot path.

PiPacketIoToIr / IrPacketIoToPd no longer copy the metadata definition maps
or the metadata definitions, only format error strings on the error path,
and move the payload into the PD message instead of copying it.

PiperOrigin-RevId: 991222288

Author: P4 Infra Team

README.md

@@ -1,4 +1,5 @@
-[![build](https://github.com/google/p4-infra/actions/workflows/ci.yml/badge.svg)](https://github.com/google/p4-infra/actions/workflows/ci.yml)
+[![build](https://github.com/google/p4-infra/actions/workflows/ci.yml/badge.svg)]
+(https://github.com/google/p4-infra/actions/workflows/ci.yml)
 
 # P4 Infrastructure
 
@@ -9,9 +10,9 @@ representation (IR) of program-dependent (PD) and program-independent (PI) and
 the code to convert between the different representations. P4Runtime generally
 uses a program-independent representation (or PI) for P4 entities such as table
 entries, counters, etc. This is achieved by using numeric IDs instead of names.
-The downside of this is that the representation is hard to read by humans. In
-contrast, a program-dependent (or PD) representation uses names and is generally
-more friendly to humans.
+The downside of this is that the representation is hard to read by humans.
+In contrast, a program-dependent (or PD) representation uses names and is
+generally more friendly to humans.
 
 The PacketLib library provides functionality to parse a raw byte string
 representation of a network packet and represent it as a collection of C++
@@ -27,13 +28,14 @@ This is not an officially supported Google product.
 The version of releases follows the year, month, date, and patch format.
 
 1.  Create a new branch with the name `release-yyyymmdd`.
-2.  In the branch, the version in the MODULE.bazel must be updated to match
-    `yyyymmdd.p`, where p is the patch number (initially 0).
+2.  In the branch, the version in the MODULE.bazel
+    must be updated to match `yyyymmdd.p`, where p is the patch number
+    (initially 0).
     *   Note that this branch should not be deleted to keep an archive of all
         releases and it makes it easier to add patches for a release.
     *   Tip: You can do steps 1 and 2 in one swoop by editing MODULE.bazel
-        directly in the browser and committing the change to a new branch of the
-        correct name.
+        directly in the browser and committing the change to a new branch of
+        the correct name.
 3.  Create a new release on Github with the tag being in the form `yyyymmdd.p`,
     the title being `P4 Infra yyyymmdd.p (month year)`, and the corresponding
     release notes (can be auto-generated).

p4_pdpi/README.md

@@ -14,8 +14,8 @@ automatically convert between them.
 
 This includes sub-libraries like (incomplete list):
 
--   [Reference Library](references.md): Validates dependencies expressed through
-    @refers_to and @referenced_by annotations across P4 entities.
+-   [Reference Library](references.md): Validates dependencies expressed
+    through @refers_to and @referenced_by annotations across P4 entities.
 
 This is a work in progress.
 

p4_pdpi/ir.cc

@@ -848,46 +848,48 @@ absl::Status PiActionSetToIr(
 // Generic helper that works for both packet-in and packet-out. For both, I is
 // one of p4::v1::{PacketIn, PacketOut} and O is one of {IrPacketIn,
 // IrPacketOut}.
+//
+// This function is on the PacketIn/PacketOut hot path, so it avoids copying
+// the metadata definitions and only formats strings on the error path.
 template <typename I, typename O>
-StatusOr<O> PiPacketIoToIr(const IrP4Info& info, const std::string& kind,
+StatusOr<O> PiPacketIoToIr(const IrP4Info& info, absl::string_view kind,
                            const I& packet, const TranslationOptions& options) {
-  O result;
-  result.set_payload(packet.payload());
-
-  const std::string& packet_description = absl::StrCat("'", kind, "' message");
-  google::protobuf::Map<uint32_t, IrPacketIoMetadataDefinition> metadata_by_id;
+  auto packet_description = [kind] {
+    return absl::StrCat("'", kind, "' message");
+  };
+  const google::protobuf::Map<uint32_t, IrPacketIoMetadataDefinition>*
+      metadata_by_id = nullptr;
   if (kind == "packet-in") {
-    metadata_by_id = info.packet_in_metadata_by_id();
+    metadata_by_id = &info.packet_in_metadata_by_id();
   } else if (kind == "packet-out") {
-    metadata_by_id = info.packet_out_metadata_by_id();
+    metadata_by_id = &info.packet_out_metadata_by_id();
   } else {
     return absl::InvalidArgumentError(GenerateFormattedError(
-        packet_description,
+        packet_description(),
         absl::StrCat(kNewBullet, "Invalid PacketIo type.")));
   }
 
+  O result;
+  result.set_payload(packet.payload());
+  result.mutable_metadata()->Reserve(packet.metadata().size());
+
   std::vector<std::string> invalid_reasons;
   absl::flat_hash_set<uint32_t> used_metadata_ids(packet.metadata().size());
   for (const auto& metadata : packet.metadata()) {
-    uint32_t id = metadata.metadata_id();
-    const absl::Status& duplicate = gutil::InsertIfUnique(
-        used_metadata_ids, id,
-        absl::StrCat("Duplicate metadata found with ID ", id, "."));
-    if (!duplicate.ok()) {
-      invalid_reasons.push_back(absl::StrCat(kNewBullet, duplicate.message()));
+    const uint32_t id = metadata.metadata_id();
+    if (!used_metadata_ids.insert(id).second) {
+      invalid_reasons.push_back(absl::StrCat(
+          kNewBullet, "Duplicate metadata found with ID ", id, "."));
       continue;
     }
 
-    const auto& status_or_metadata_definition_ptr =
-        gutil::FindPtrOrStatus(metadata_by_id, id);
-    if (!status_or_metadata_definition_ptr.ok()) {
+    const auto it = metadata_by_id->find(id);
+    if (it == metadata_by_id->end()) {
       invalid_reasons.push_back(
           absl::StrCat(kNewBullet, " Metadata with ID ", id, " not defined."));
       continue;
     }
-
-    const pdpi::IrPacketIoMetadataDefinition metadata_definition =
-        **status_or_metadata_definition_ptr;
+    const pdpi::IrPacketIoMetadataDefinition& metadata_definition = it->second;
 
     // Metadata with @padding annotation must be all zeros and must not be
     // included in IR representation.
@@ -900,35 +902,39 @@ StatusOr<O> PiPacketIoToIr(const IrP4Info& info, const std::string& kind,
       continue;
     }
 
-    IrPacketMetadata ir_metadata;
     const std::string& metadata_name = metadata_definition.metadata().name();
-    ir_metadata.set_name(metadata_name);
-    const absl::StatusOr<IrValue> ir_value = ArbitraryByteStringToIrValue(
+    absl::StatusOr<IrValue> ir_value = ArbitraryByteStringToIrValue(
         metadata_definition.format(), metadata_definition.metadata().bitwidth(),
         metadata.value());
     if (!ir_value.ok()) {
       invalid_reasons.push_back(GenerateReason(MetadataName(metadata_name),
                                                ir_value.status().message()));
       continue;
     }
-    *ir_metadata.mutable_value() = *ir_value;
-    *result.add_metadata() = ir_metadata;
-  }
-  // Check for missing metadata
-  for (const auto& item : metadata_by_id) {
-    const auto& id = item.first;
-    const auto& meta = item.second;
-    if (!used_metadata_ids.contains(id)) {
-      invalid_reasons.push_back(
-          absl::StrCat(kNewBullet, "Metadata '", meta.metadata().name(),
-                       "' with id ", meta.metadata().id(), " is missing."));
-      continue;
+    IrPacketMetadata* ir_metadata = result.add_metadata();
+    ir_metadata->set_name(metadata_name);
+    *ir_metadata->mutable_value() = *std::move(ir_value);
+  }
+  // Check for missing metadata. If no errors occurred so far, then every ID in
+  // `used_metadata_ids` is unique (duplicates are rejected above) and defined
+  // in `metadata_by_id` (undefined IDs are rejected above), so
+  // `used_metadata_ids` is a subset of the defined IDs. Matching sizes
+  // therefore imply the two sets are equal, i.e. nothing is missing, and the
+  // full scan below is only needed on the error path.
+  if (used_metadata_ids.size() != metadata_by_id->size() ||
+      !invalid_reasons.empty()) {
+    for (const auto& [id, meta] : *metadata_by_id) {
+      if (!used_metadata_ids.contains(id)) {
+        invalid_reasons.push_back(
+            absl::StrCat(kNewBullet, "Metadata '", meta.metadata().name(),
+                         "' with id ", meta.metadata().id(), " is missing."));
+      }
     }
   }
 
   if (!invalid_reasons.empty()) {
     return absl::InvalidArgumentError(GenerateFormattedError(
-        packet_description, absl::StrJoin(invalid_reasons, "\n")));
+        packet_description(), absl::StrJoin(invalid_reasons, "\n")));
   }
 
   return result;

p4_pdpi/ir_utils.cc

@@ -230,15 +230,16 @@ absl::StatusOr<IrValue> ArbitraryByteStringToIrValue(Format format,
     case Format::HEX_STRING: {
       ASSIGN_OR_RETURN(std::string normalized_bytes,
                        ArbitraryToNormalizedByteString(bytes, bitwidth));
-      std::string hex_string = absl::BytesToHexString(normalized_bytes);
       const int expected_num_hex_chars =
           bitwidth / 4 + (bitwidth % 4 != 0 ? 1 : 0);
-      if (expected_num_hex_chars != hex_string.size()) {
+      const std::string hex_string = absl::BytesToHexString(normalized_bytes);
+      absl::string_view hex_view = hex_string;
+      if (expected_num_hex_chars != hex_view.size()) {
         // absl::BytesToHexString operates on bytes (= 8 bits), but we want to
         // operate on nibbles (= 4 bits). This fixes the length as necessary.
-        hex_string = hex_string.substr(1);
+        hex_view.remove_prefix(1);
       }
-      result.set_hex_str(absl::StrCat("0x", hex_string));
+      result.set_hex_str(absl::StrCat("0x", hex_view));
       return result;
     }
     default:

p4_pdpi/pd.cc

@@ -351,7 +351,8 @@ absl::Status SetStringField(google::protobuf::Message* message,
                    GetFieldDescriptor(*message, fieldname));
   RETURN_IF_ERROR(ValidateFieldDescriptorType(field_descriptor,
                                               FieldDescriptor::TYPE_STRING));
-  message->GetReflection()->SetString(message, field_descriptor, value);
+  message->GetReflection()->SetString(message, field_descriptor,
+                                      std::move(value));
   return absl::OkStatus();
 }
 
@@ -361,7 +362,8 @@ absl::Status SetBytesField(google::protobuf::Message* message,
                    GetFieldDescriptor(*message, fieldname));
   RETURN_IF_ERROR(ValidateFieldDescriptorType(field_descriptor,
                                               FieldDescriptor::TYPE_BYTES));
-  message->GetReflection()->SetString(message, field_descriptor, value);
+  message->GetReflection()->SetString(message, field_descriptor,
+                                      std::move(value));
   return absl::OkStatus();
 }
 
@@ -631,20 +633,32 @@ absl::Status IrTableEntriesToPd(const IrP4Info& ir_p4info,
   return absl::OkStatus();
 }
 
+// Defined below. See definition for details.
+template <typename T>
+absl::Status IrPacketIoToPd(const IrP4Info& info, absl::string_view kind,
+                            const T& packet,
+                            google::protobuf::Message* pd_packet,
+                            const TranslationOptions& options,
+                            std::string* movable_payload = nullptr);
+
 absl::Status PiPacketInToPd(const IrP4Info& info,
                             const p4::v1::PacketIn& pi_packet,
                             google::protobuf::Message* pd_packet,
                             const TranslationOptions& options) {
-  ASSIGN_OR_RETURN(const auto ir, PiPacketInToIr(info, pi_packet, options));
-  return IrPacketInToPd(info, ir, pd_packet, options);
+  ASSIGN_OR_RETURN(IrPacketIn ir, PiPacketInToIr(info, pi_packet, options));
+  // `ir` is a temporary, so its payload can be moved rather than copied.
+  return IrPacketIoToPd<IrPacketIn>(info, "packet-in", ir, pd_packet, options,
+                                    ir.mutable_payload());
 }
 
 absl::Status PiPacketOutToPd(const IrP4Info& info,
                              const p4::v1::PacketOut& pi_packet,
                              google::protobuf::Message* pd_packet,
                              const TranslationOptions& options) {
-  ASSIGN_OR_RETURN(const auto ir, PiPacketOutToIr(info, pi_packet, options));
-  return IrPacketOutToPd(info, ir, pd_packet, options);
+  ASSIGN_OR_RETURN(IrPacketOut ir, PiPacketOutToIr(info, pi_packet, options));
+  // `ir` is a temporary, so its payload can be moved rather than copied.
+  return IrPacketIoToPd<IrPacketOut>(info, "packet-out", ir, pd_packet, options,
+                                     ir.mutable_payload());
 }
 
 absl::Status PiReadRequestToPd(const IrP4Info& info,
@@ -1325,54 +1339,74 @@ absl::Status IrTableEntryToPd(const IrP4Info& ir_p4info, const IrTableEntry& ir,
   return absl::OkStatus();
 }
 
+// Generic helper for packet-in and packet-out. If `movable_payload` is
+// non-null, it must point to `packet.payload()` and its contents are moved into
+// `pd_packet` instead of being copied.
+//
+// This function is on the PacketIn/PacketOut hot path, so it avoids copying
+// the metadata definitions and only formats strings on the error path.
 template <typename T>
-absl::Status IrPacketIoToPd(const IrP4Info& info, const std::string& kind,
+absl::Status IrPacketIoToPd(const IrP4Info& info, absl::string_view kind,
                             const T& packet,
                             google::protobuf::Message* pd_packet,
-                            const TranslationOptions& options) {
-  const std::string& packet_description = absl::StrCat("'", kind, "' message");
-  const auto& field_descriptor = GetFieldDescriptor(*pd_packet, "payload");
-  if (!field_descriptor.ok()) {
+                            const TranslationOptions& options,
+                            std::string* movable_payload) {
+  auto packet_description = [kind] {
+    return absl::StrCat("'", kind, "' message");
+  };
+  const google::protobuf::Map<std::string, IrPacketIoMetadataDefinition>*
+      metadata_by_name = nullptr;
+  if (kind == "packet-in") {
+    metadata_by_name = &info.packet_in_metadata_by_name();
+  } else if (kind == "packet-out") {
+    metadata_by_name = &info.packet_out_metadata_by_name();
+  } else {
     return absl::InvalidArgumentError(GenerateFormattedError(
-        packet_description,
-        absl::StrCat(kNewBullet, field_descriptor.status().message())));
+        packet_description(),
+        absl::StrCat(kNewBullet, "Invalid PacketIo type.")));
   }
-  const auto& validate_status = ValidateFieldDescriptorType(
-      *field_descriptor, FieldDescriptor::TYPE_BYTES);
+
+  const google::protobuf::Descriptor* pd_descriptor =
+      pd_packet->GetDescriptor();
+  const google::protobuf::Reflection* pd_reflection =
+      pd_packet->GetReflection();
+  const FieldDescriptor* payload_field =
+      pd_descriptor->FindFieldByName("payload");
+  if (payload_field == nullptr) {
+    return absl::InvalidArgumentError(GenerateFormattedError(
+        packet_description(),
+        absl::StrCat(kNewBullet, "Field payload missing in ",
+                     pd_packet->GetTypeName())));
+  }
+  const auto& validate_status =
+      ValidateFieldDescriptorType(payload_field, FieldDescriptor::TYPE_BYTES);
   if (!validate_status.ok()) {
     return absl::InvalidArgumentError(GenerateFormattedError(
-        packet_description,
+        packet_description(),
         absl::StrCat(kNewBullet, validate_status.message())));
   }
-  pd_packet->GetReflection()->SetString(pd_packet, *field_descriptor,
-                                        packet.payload());
-
-  google::protobuf::Map<std::string, IrPacketIoMetadataDefinition>
-      metadata_by_name;
-  if (kind == "packet-in") {
-    metadata_by_name = info.packet_in_metadata_by_name();
-  } else if (kind == "packet-out") {
-    metadata_by_name = info.packet_out_metadata_by_name();
+  if (movable_payload != nullptr) {
+    pd_reflection->SetString(pd_packet, payload_field,
+                             std::move(*movable_payload));
   } else {
-    return absl::InvalidArgumentError(GenerateFormattedError(
-        packet_description,
-        absl::StrCat(kNewBullet, "Invalid PacketIo type.")));
+    pd_reflection->SetString(pd_packet, payload_field, packet.payload());
   }
 
   std::vector<std::string> invalid_reasons;
+  // Looked up lazily (once) so that packets without metadata leave the PD
+  // `metadata` field unset, as before.
+  absl::StatusOr<google::protobuf::Message*> pd_metadata;
+  bool pd_metadata_initialized = false;
   for (const auto& metadata : packet.metadata()) {
     const std::string& name = metadata.name();
 
-    const auto& status_or_metadata_definition =
-        gutil::FindPtrOrStatus(metadata_by_name, name);
-    if (!status_or_metadata_definition.ok()) {
+    const auto it = metadata_by_name->find(name);
+    if (it == metadata_by_name->end()) {
       invalid_reasons.push_back(absl::StrCat(kNewBullet, "Metadata with name '",
                                              name, "' not defined."));
       continue;
     }
-
-    const pdpi::IrPacketIoMetadataDefinition& metadata_definition =
-        **status_or_metadata_definition;
+    const pdpi::IrPacketIoMetadataDefinition& metadata_definition = it->second;
 
     if (metadata_definition.is_padding()) {
       invalid_reasons.push_back(absl::StrCat(
@@ -1382,22 +1416,17 @@ absl::Status IrPacketIoToPd(const IrP4Info& info, const std::string& kind,
       continue;
     }
 
-    const absl::StatusOr<std::string>& raw_value =
-        IrValueString(metadata.value());
-    if (!raw_value.ok()) {
-      invalid_reasons.push_back(
-          GenerateReason(MetadataName(name), raw_value.status().message()));
-      continue;
+    if (!pd_metadata_initialized) {
+      pd_metadata = GetMutableMessage(pd_packet, "metadata");
+      pd_metadata_initialized = true;
     }
-    const absl::StatusOr<google::protobuf::Message*>& pd_metadata =
-        GetMutableMessage(pd_packet, "metadata");
     if (!pd_metadata.ok()) {
       invalid_reasons.push_back(
           GenerateReason(MetadataName(name), pd_metadata.status().message()));
       continue;
     }
-    const absl::Status& value_status =
-        SetStringField(*pd_metadata, name, *raw_value);
+    const absl::Status value_status =
+        SetStringField(*pd_metadata, name, IrValueString(metadata.value()));
     if (!value_status.ok()) {
       invalid_reasons.push_back(
           GenerateReason(MetadataName(name), value_status.message()));
@@ -1406,7 +1435,7 @@ absl::Status IrPacketIoToPd(const IrP4Info& info, const std::string& kind,
   }
   if (!invalid_reasons.empty()) {
     return absl::InvalidArgumentError(GenerateFormattedError(
-        packet_description, absl::StrJoin(invalid_reasons, "\n")));
+        packet_description(), absl::StrJoin(invalid_reasons, "\n")));
   }
   return absl::OkStatus();
 }