From 205a7ea8b16bf430f2040e973f9add7c8f98f178 Mon Sep 17 00:00:00 2001 From: Joshua Haberman Date: Sat, 5 Feb 2022 21:29:27 -0800 Subject: [PATCH 1/3] Added a variant of the def-loading benchmark that builds layout dynamically. This will help us evaluate the performance impacts (if any) that the new mini-table building code will have. --- benchmarks/benchmark.cc | 123 ++++++++++++++++++++++++++++------------ 1 file changed, 86 insertions(+), 37 deletions(-) diff --git a/benchmarks/benchmark.cc b/benchmarks/benchmark.cc index 06c16fd91a..aedd8bf0be 100644 --- a/benchmarks/benchmark.cc +++ b/benchmarks/benchmark.cc @@ -33,6 +33,7 @@ #include "benchmarks/descriptor_sv.pb.h" #include "google/ads/googleads/v7/services/google_ads_service.upbdefs.h" #include "google/protobuf/descriptor.pb.h" +#include "google/protobuf/dynamic_message.h" #include "upb/def.hpp" upb_StringView descriptor = benchmarks_descriptor_proto_upbdefinit.descriptor; @@ -70,47 +71,90 @@ static void BM_ArenaInitialBlockOneAlloc(benchmark::State& state) { } BENCHMARK(BM_ArenaInitialBlockOneAlloc); -static void BM_LoadDescriptor_Upb(benchmark::State& state) { - size_t bytes_per_iter = 0; - for (auto _ : state) { - upb::SymbolTable symtab; - upb_benchmark_DescriptorProto_getmsgdef(symtab.ptr()); - bytes_per_iter = _upb_DefPool_BytesLoaded(symtab.ptr()); +enum LoadDescriptorMode { + NoLayout, + WithLayout, +}; + +// This function is mostly copied from upb/def.c, but it is modified to avoid +// passing in the pre-generated mini-tables, in order to force upb to compute +// them dynamically. Generally you would never want to do this, but we want to +// simulate the cost we would pay if we were loading these types purely from +// descriptors, with no mini-tales available. +bool LoadDefInit_NoLayout(upb_DefPool *s, const _upb_DefPool_Init *init, + size_t *bytes) { + _upb_DefPool_Init** deps = init->deps; + google_protobuf_FileDescriptorProto* file; + upb_Arena* arena; + upb_Status status; + + upb_Status_Clear(&status); + + if (upb_DefPool_FindFileByName(s, init->filename)) { + return true; } - state.SetBytesProcessed(state.iterations() * bytes_per_iter); + + arena = upb_Arena_New(); + + for (; *deps; deps++) { + if (!LoadDefInit_NoLayout(s, *deps, bytes)) + goto err; + } + + file = google_protobuf_FileDescriptorProto_parse_ex( + init->descriptor.data, init->descriptor.size, NULL, + kUpb_DecodeOption_AliasString, arena); + *bytes += init->descriptor.size; + + if (!file) { + upb_Status_SetErrorFormat( + &status, + "Failed to parse compiled-in descriptor for file '%s'. This should " + "never happen.", + init->filename); + goto err; + } + + // KEY DIFFERENCE: Here we pass in only the descriptor, and not the + // pre-generated minitables. + if (!upb_DefPool_AddFile(s, file, &status)) { + goto err; + } + + upb_Arena_Free(arena); + return true; + +err: + fprintf(stderr, + "Error loading compiled-in descriptor for file '%s' (this should " + "never happen): %s\n", + init->filename, upb_Status_ErrorMessage(&status)); + exit(1); } -BENCHMARK(BM_LoadDescriptor_Upb); +template static void BM_LoadAdsDescriptor_Upb(benchmark::State& state) { size_t bytes_per_iter = 0; for (auto _ : state) { upb::SymbolTable symtab; - google_ads_googleads_v7_services_SearchGoogleAdsRequest_getmsgdef( - symtab.ptr()); - bytes_per_iter = _upb_DefPool_BytesLoaded(symtab.ptr()); - } - state.SetBytesProcessed(state.iterations() * bytes_per_iter); -} -BENCHMARK(BM_LoadAdsDescriptor_Upb); - -static void BM_LoadDescriptor_Proto2(benchmark::State& state) { - for (auto _ : state) { - protobuf::Arena arena; - protobuf::StringPiece input(descriptor.data, descriptor.size); - auto proto = - protobuf::Arena::CreateMessage(&arena); - protobuf::DescriptorPool pool; - bool ok = proto->ParseFrom(input) && - pool.BuildFile(*proto) != nullptr; - if (!ok) { - printf("Failed to add file.\n"); - exit(1); + if (Mode == NoLayout) { + google_ads_googleads_v7_services_SearchGoogleAdsRequest_getmsgdef( + symtab.ptr()); + bytes_per_iter = _upb_DefPool_BytesLoaded(symtab.ptr()); + } else { + bytes_per_iter = 0; + LoadDefInit_NoLayout( + symtab.ptr(), + &google_ads_googleads_v7_services_google_ads_service_proto_upbdefinit, + &bytes_per_iter); } } - state.SetBytesProcessed(state.iterations() * descriptor.size); + state.SetBytesProcessed(state.iterations() * bytes_per_iter); } -BENCHMARK(BM_LoadDescriptor_Proto2); +BENCHMARK_TEMPLATE(BM_LoadAdsDescriptor_Upb, NoLayout); +BENCHMARK_TEMPLATE(BM_LoadAdsDescriptor_Upb, WithLayout); +template static void BM_LoadAdsDescriptor_Proto2(benchmark::State& state) { extern _upb_DefPool_Init google_ads_googleads_v7_services_google_ads_service_proto_upbdefinit; @@ -136,10 +180,21 @@ static void BM_LoadAdsDescriptor_Proto2(benchmark::State& state) { } bytes_per_iter += input.size(); } + + if (Mode == WithLayout) { + protobuf::DynamicMessageFactory factory; + const protobuf::Descriptor* d = pool.FindMessageTypeByName("google.ads.googleads.v7.services.SearchGoogleAdsResponse"); + if (!d) { + printf("Failed to find descriptor.\n"); + exit(1); + } + factory.GetPrototype(d); + } } state.SetBytesProcessed(state.iterations() * bytes_per_iter); } -BENCHMARK(BM_LoadAdsDescriptor_Proto2); +BENCHMARK_TEMPLATE(BM_LoadAdsDescriptor_Proto2, NoLayout); +BENCHMARK_TEMPLATE(BM_LoadAdsDescriptor_Proto2, WithLayout); enum CopyStrings { Copy, @@ -154,7 +209,6 @@ enum ArenaMode { template static void BM_Parse_Upb_FileDesc(benchmark::State& state) { - size_t bytes = 0; for (auto _ : state) { upb_Arena* arena; if (AMode == InitBlock) { @@ -170,7 +224,6 @@ static void BM_Parse_Upb_FileDesc(benchmark::State& state) { printf("Failed to parse.\n"); exit(1); } - bytes += descriptor.size; upb_Arena_Free(arena); } state.SetBytesProcessed(state.iterations() * descriptor.size); @@ -223,7 +276,6 @@ using FileDescSV = ::upb_benchmark::sv::FileDescriptorProto; template void BM_Parse_Proto2(benchmark::State& state) { - size_t bytes = 0; constexpr protobuf::MessageLite::ParseFlags kParseFlags = kCopy == Copy ? protobuf::MessageLite::ParseFlags::kMergePartial @@ -237,7 +289,6 @@ void BM_Parse_Proto2(benchmark::State& state) { printf("Failed to parse.\n"); exit(1); } - bytes += descriptor.size; } state.SetBytesProcessed(state.iterations() * descriptor.size); } @@ -247,12 +298,10 @@ BENCHMARK_TEMPLATE(BM_Parse_Proto2, FileDesc, InitBlock, Copy); BENCHMARK_TEMPLATE(BM_Parse_Proto2, FileDescSV, InitBlock, Alias); static void BM_SerializeDescriptor_Proto2(benchmark::State& state) { - size_t bytes = 0; upb_benchmark::FileDescriptorProto proto; proto.ParseFromArray(descriptor.data, descriptor.size); for (auto _ : state) { proto.SerializePartialToArray(buf, sizeof(buf)); - bytes += descriptor.size; } state.SetBytesProcessed(state.iterations() * descriptor.size); } From 4c9891bf3ecba84a018222a2123eea63647eee78 Mon Sep 17 00:00:00 2001 From: Joshua Haberman Date: Sat, 5 Feb 2022 22:26:22 -0800 Subject: [PATCH 2/3] Renamed LoadDefInit_NoLayout() to LoadDefInit_BuildLayout(). This will clarify that the function should go with the WithLayout benchmarks. --- benchmarks/benchmark.cc | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/benchmarks/benchmark.cc b/benchmarks/benchmark.cc index aedd8bf0be..e02dbd3a42 100644 --- a/benchmarks/benchmark.cc +++ b/benchmarks/benchmark.cc @@ -81,8 +81,8 @@ enum LoadDescriptorMode { // them dynamically. Generally you would never want to do this, but we want to // simulate the cost we would pay if we were loading these types purely from // descriptors, with no mini-tales available. -bool LoadDefInit_NoLayout(upb_DefPool *s, const _upb_DefPool_Init *init, - size_t *bytes) { +bool LoadDefInit_BuildLayout(upb_DefPool *s, const _upb_DefPool_Init *init, + size_t *bytes) { _upb_DefPool_Init** deps = init->deps; google_protobuf_FileDescriptorProto* file; upb_Arena* arena; @@ -97,7 +97,7 @@ bool LoadDefInit_NoLayout(upb_DefPool *s, const _upb_DefPool_Init *init, arena = upb_Arena_New(); for (; *deps; deps++) { - if (!LoadDefInit_NoLayout(s, *deps, bytes)) + if (!LoadDefInit_BuildLayout(s, *deps, bytes)) goto err; } @@ -143,7 +143,7 @@ static void BM_LoadAdsDescriptor_Upb(benchmark::State& state) { bytes_per_iter = _upb_DefPool_BytesLoaded(symtab.ptr()); } else { bytes_per_iter = 0; - LoadDefInit_NoLayout( + LoadDefInit_BuildLayout( symtab.ptr(), &google_ads_googleads_v7_services_google_ads_service_proto_upbdefinit, &bytes_per_iter); From 6af01a26a0efe735c347257d61631bd6bb2a030f Mon Sep 17 00:00:00 2001 From: Joshua Haberman Date: Sat, 5 Feb 2022 22:29:49 -0800 Subject: [PATCH 3/3] Ran clang-format. --- benchmarks/benchmark.cc | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/benchmarks/benchmark.cc b/benchmarks/benchmark.cc index e02dbd3a42..368780e1f3 100644 --- a/benchmarks/benchmark.cc +++ b/benchmarks/benchmark.cc @@ -81,8 +81,8 @@ enum LoadDescriptorMode { // them dynamically. Generally you would never want to do this, but we want to // simulate the cost we would pay if we were loading these types purely from // descriptors, with no mini-tales available. -bool LoadDefInit_BuildLayout(upb_DefPool *s, const _upb_DefPool_Init *init, - size_t *bytes) { +bool LoadDefInit_BuildLayout(upb_DefPool* s, const _upb_DefPool_Init* init, + size_t* bytes) { _upb_DefPool_Init** deps = init->deps; google_protobuf_FileDescriptorProto* file; upb_Arena* arena; @@ -97,8 +97,7 @@ bool LoadDefInit_BuildLayout(upb_DefPool *s, const _upb_DefPool_Init *init, arena = upb_Arena_New(); for (; *deps; deps++) { - if (!LoadDefInit_BuildLayout(s, *deps, bytes)) - goto err; + if (!LoadDefInit_BuildLayout(s, *deps, bytes)) goto err; } file = google_protobuf_FileDescriptorProto_parse_ex( @@ -183,7 +182,8 @@ static void BM_LoadAdsDescriptor_Proto2(benchmark::State& state) { if (Mode == WithLayout) { protobuf::DynamicMessageFactory factory; - const protobuf::Descriptor* d = pool.FindMessageTypeByName("google.ads.googleads.v7.services.SearchGoogleAdsResponse"); + const protobuf::Descriptor* d = pool.FindMessageTypeByName( + "google.ads.googleads.v7.services.SearchGoogleAdsResponse"); if (!d) { printf("Failed to find descriptor.\n"); exit(1);