Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

avoid allocating memory if all extension are cleared #9345

Merged
merged 5 commits into from
Mar 10, 2022

Conversation

liuzhijiang
Copy link
Contributor

avoid allocating memory if all extension are cleared

avoid allocating memory if all extension are cleared
@liuzhijiang liuzhijiang reopened this Dec 26, 2021
@sbenzaquen
Copy link
Contributor

We should test that the capacity grows as expected with the new changes for is_cleared.

src/google/protobuf/extension_set.cc Outdated Show resolved Hide resolved
use the 4x growth.
@acozzette
Copy link
Member

@liuzhijiang This change looks good to me but I think we just need a unit test as @sbenzaquen suggested to verify the new behavior. I think the test could just look at SpaceUsedExcludingSelfLong() and verify that this number doesn't go up when we merge an ExtensionSet that has been cleared.

@liuzhijiang
Copy link
Contributor Author

liuzhijiang commented Feb 25, 2022

The function Reflection::GetExtensionSet() is private, so the object ExtensionSet cannot be obtained and the function ExtensionSet::SpaceUsedExcludingSelfLong() cannot be called. Is there any other way to call the function SpaceUsedExcludingSelfLong() ?
If there is no other way to call this function, is the following unit test OK?

TEST(ExtensionSetTest, ExtensionSetSpaceUsed) {
  unittest::TestAllExtensions msg;
  long l = msg.SpaceUsedLong();
  msg.SetExtension(unittest::optional_int32_extension, 100);
  unittest::TestAllExtensions msg2(msg);
  long l2 = msg2.SpaceUsedLong();
  msg.ClearExtension(unittest::optional_int32_extension);
  unittest::TestAllExtensions msg3(msg);
  long l3 = msg3.SpaceUsedLong();
  EXPECT_TRUE((l2 - l) > (l3 - l));
}

@acozzette
Copy link
Member

@liuzhijiang That kind of unit test looks good to me. I think you are right that the best way to do it is to rely on Message::SpaceUsedLong() since ExtensionSet::SpaceUsedLong() is not easily visible for testing.

@acozzette acozzette merged commit 7d5af9d into protocolbuffers:master Mar 10, 2022
@acozzette
Copy link
Member

@liuzhijiang Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants