Skip to content

Commit 3396e5e

Browse files
Improve MemoizeProjection by allowing any kind of descriptor.
Also, make it a static function to prevent bugs where the pool and the field don't match. We can get the pool from the field anyway. PiperOrigin-RevId: 741531141
1 parent 018dc02 commit 3396e5e

File tree

3 files changed

+50
-26
lines changed

3 files changed

+50
-26
lines changed

src/google/protobuf/descriptor.h

Lines changed: 15 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -2527,28 +2527,30 @@ class PROTOBUF_EXPORT DescriptorPool {
25272527
T value;
25282528
};
25292529

2530-
// Memoize a projection of a field. This is used to cache the results of
2531-
// calling a function on a field, used for expensive descriptor calculations.
2532-
template <typename Func>
2533-
const auto& MemoizeProjection(const FieldDescriptor* field, Func func) const {
2534-
using ResultT = std::decay_t<decltype(func(field))>;
2535-
ABSL_DCHECK(field->file()->pool() == this);
2530+
// Memoize a projection of a descriptor. This is used to cache the results of
2531+
// calling a function on a descriptor, used for expensive descriptor
2532+
// calculations.
2533+
template <typename Desc, typename Func>
2534+
static const auto& MemoizeProjection(const Desc* descriptor, Func func) {
2535+
using ResultT = std::decay_t<decltype(func(descriptor))>;
2536+
auto* pool = descriptor->file()->pool();
25362537
static_assert(std::is_empty_v<Func>);
25372538
// This static bool is unique per-Func, so its address can be used as a key.
25382539
static bool type_key;
2539-
auto key = std::pair<const void*, const void*>(field, &type_key);
2540+
auto key = std::pair<const void*, const void*>(descriptor, &type_key);
25402541
{
2541-
absl::ReaderMutexLock lock(&field_memo_table_mutex_);
2542-
auto it = field_memo_table_->find(key);
2543-
if (it != field_memo_table_->end()) {
2542+
absl::ReaderMutexLock lock(&pool->field_memo_table_mutex_);
2543+
auto it = pool->field_memo_table_->find(key);
2544+
if (it != pool->field_memo_table_->end()) {
25442545
return internal::DownCast<const MemoData<ResultT>&>(*it->second).value;
25452546
}
25462547
}
25472548
auto result = std::make_unique<MemoData<ResultT>>();
2548-
result->value = func(field);
2549+
result->value = func(descriptor);
25492550
{
2550-
absl::MutexLock lock(&field_memo_table_mutex_);
2551-
auto insert_result = field_memo_table_->insert({key, std::move(result)});
2551+
absl::MutexLock lock(&pool->field_memo_table_mutex_);
2552+
auto insert_result =
2553+
pool->field_memo_table_->insert({key, std::move(result)});
25522554
auto it = insert_result.first;
25532555
return internal::DownCast<const MemoData<ResultT>&>(*it->second).value;
25542556
}

src/google/protobuf/descriptor_unittest.cc

Lines changed: 34 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -12230,10 +12230,9 @@ TEST_F(DescriptorPoolFeaturesTest, ResolvesFeaturesFor) {
1223012230

1223112231
class DescriptorPoolMemoizationTest : public ::testing::Test {
1223212232
protected:
12233-
template <typename Func>
12234-
const auto& MemoizeProjection(const DescriptorPool* pool,
12235-
const FieldDescriptor* field, Func func) {
12236-
return pool->MemoizeProjection(field, func);
12233+
template <typename Desc, typename Func>
12234+
const auto& MemoizeProjection(const Desc* descriptor, Func func) {
12235+
return DescriptorPool::MemoizeProjection(descriptor, func);
1223712236
};
1223812237
};
1223912238

@@ -12243,13 +12242,12 @@ TEST_F(DescriptorPoolMemoizationTest, MemoizeProjectionBasic) {
1224312242
counter++;
1224412243
return field->full_name();
1224512244
};
12246-
proto2_unittest::TestAllTypes message;
12247-
const Descriptor* descriptor = message.GetDescriptor();
12245+
const Descriptor* descriptor = proto2_unittest::TestAllTypes::descriptor();
1224812246

1224912247
const auto& name = DescriptorPoolMemoizationTest::MemoizeProjection(
12250-
descriptor->file()->pool(), descriptor->field(0), name_lambda);
12248+
descriptor->field(0), name_lambda);
1225112249
const auto& dupe_name = DescriptorPoolMemoizationTest::MemoizeProjection(
12252-
descriptor->file()->pool(), descriptor->field(0), name_lambda);
12250+
descriptor->field(0), name_lambda);
1225312251

1225412252
ASSERT_EQ(counter, 1);
1225512253
ASSERT_EQ(name, "proto2_unittest.TestAllTypes.optional_int32");
@@ -12261,12 +12259,36 @@ TEST_F(DescriptorPoolMemoizationTest, MemoizeProjectionBasic) {
1226112259
EXPECT_EQ(&name, &dupe_name);
1226212260

1226312261
auto other_name = DescriptorPoolMemoizationTest::MemoizeProjection(
12264-
descriptor->file()->pool(), descriptor->field(1), name_lambda);
12262+
descriptor->field(1), name_lambda);
1226512263

1226612264
ASSERT_EQ(counter, 2);
1226712265
ASSERT_NE(other_name, "proto2_unittest.TestAllTypes.optional_int32");
1226812266
}
1226912267

12268+
TEST_F(DescriptorPoolMemoizationTest, SupportsDifferentDescriptorTypes) {
12269+
static int counter;
12270+
counter = 0;
12271+
auto name_lambda = [](const auto* field) {
12272+
counter++;
12273+
return field->full_name();
12274+
};
12275+
12276+
const Descriptor* descriptor = proto2_unittest::TestAllTypes::descriptor();
12277+
12278+
// Different descriptor types should be accepted and return the appropriate
12279+
// result, even when reusing the same lambda type.
12280+
EXPECT_EQ("proto2_unittest.TestAllTypes.optional_int32",
12281+
DescriptorPoolMemoizationTest::MemoizeProjection(
12282+
descriptor->field(0), name_lambda));
12283+
EXPECT_EQ("proto2_unittest.TestAllTypes",
12284+
DescriptorPoolMemoizationTest::MemoizeProjection(descriptor,
12285+
name_lambda));
12286+
EXPECT_EQ("proto2_unittest.TestAllTypes.NestedMessage",
12287+
DescriptorPoolMemoizationTest::MemoizeProjection(
12288+
descriptor->nested_type(0), name_lambda));
12289+
EXPECT_EQ(counter, 3);
12290+
}
12291+
1227012292
TEST_F(DescriptorPoolMemoizationTest, MemoizeProjectionMultithreaded) {
1227112293
auto name_lambda = [](const FieldDescriptor* field) {
1227212294
return field->full_name();
@@ -12277,9 +12299,9 @@ TEST_F(DescriptorPoolMemoizationTest, MemoizeProjectionMultithreaded) {
1227712299
for (int i = 0; i < descriptor->field_count(); ++i) {
1227812300
threads.emplace_back([this, name_lambda, descriptor, i]() {
1227912301
auto name = DescriptorPoolMemoizationTest::MemoizeProjection(
12280-
descriptor->file()->pool(), descriptor->field(i), name_lambda);
12302+
descriptor->field(i), name_lambda);
1228112303
auto first_name = DescriptorPoolMemoizationTest::MemoizeProjection(
12282-
descriptor->file()->pool(), descriptor->field(0), name_lambda);
12304+
descriptor->field(0), name_lambda);
1228312305
ASSERT_THAT(name, HasSubstr("proto2_unittest.TestAllTypes"));
1228412306
if (i != 0) {
1228512307
ASSERT_NE(name, "proto2_unittest.TestAllTypes.optional_int32");
@@ -12303,7 +12325,7 @@ TEST_F(DescriptorPoolMemoizationTest, MemoizeProjectionInsertionRace) {
1230312325
for (int j = 0; j < 3; ++j) {
1230412326
threads.emplace_back([this, name_lambda, descriptor, i]() {
1230512327
auto name = DescriptorPoolMemoizationTest::MemoizeProjection(
12306-
descriptor->file()->pool(), descriptor->field(i), name_lambda);
12328+
descriptor->field(i), name_lambda);
1230712329
ASSERT_THAT(name, HasSubstr("proto2_unittest.TestAllTypes"));
1230812330
});
1230912331
}

src/google/protobuf/text_format.cc

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3037,7 +3037,7 @@ bool TextFormat::Printer::TryRedactFieldValue(
30373037
const Message& message, const FieldDescriptor* field,
30383038
BaseTextGenerator* generator, bool insert_value_separator) const {
30393039
TextFormat::RedactionState redaction_state =
3040-
field->file()->pool()->MemoizeProjection(
3040+
DescriptorPool::MemoizeProjection(
30413041
field, [](const FieldDescriptor* field) {
30423042
return TextFormat::GetRedactionState(field);
30433043
});

0 commit comments

Comments
 (0)