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

Support arena allocation of google::protobuf::AnyMetadata #8758

Merged
merged 1 commit into from Jul 12, 2021

Conversation

georgthegreat
Copy link
Contributor

@georgthegreat georgthegreat commented Jun 23, 2021

Fixes #8264

@google-cla google-cla bot added the cla: yes label Jun 23, 2021
@@ -99,7 +99,7 @@ class Any::_Internal {
Any::Any(::PROTOBUF_NAMESPACE_ID::Arena* arena,
bool is_message_owned)
: ::PROTOBUF_NAMESPACE_ID::Message(arena, is_message_owned),
_any_metadata_(&type_url_, &value_) {
_any_metadata_(arena, &type_url_, &value_) {
Copy link
Contributor Author

@georgthegreat georgthegreat Jun 23, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NB:

Local regeneration of any.pb.cc from any.proto removes some PROTOBUF_EXPORT statements from any.pb.h.
I did not included these changes into PR.

@georgthegreat georgthegreat changed the title Spupport arena allocation of google::protobuf::AnyMetadata Support arena allocation of google::protobuf::AnyMetadata Jun 23, 2021
@acozzette acozzette added release notes: yes Include this PR description in the next release c++ kokoro:run kokoro:force-run labels Jun 25, 2021
@acozzette
Copy link
Member

acozzette commented Jun 28, 2021

@georgthegreat Thanks for the fix! Looks pretty good but I have just a couple comments:

  1. I think we might as well pass Arena* as an argument to AnyMetadata::PackFrom() instead of storing it as a member variable. google::protobuf::Any has an arena member already, so this way we don't have to store it in two places.
  2. The fix has to appear in any_lite.cc in addition to any.cc.

@georgthegreat
Copy link
Contributor Author

georgthegreat commented Jul 9, 2021

@acozzette, this PR should fix the problem in a proper way.

Unfortunately, running protoc --cpp_out . any.proto turns any.pb.cc into incompilable code, hence I am unable to check if code generation is ever correct.

Patches in protobuf/compiler might be incorrect.

@acozzette acozzette merged commit 7b1f793 into protocolbuffers:master Jul 12, 2021
52 of 54 checks passed
@georgthegreat georgthegreat deleted the any-arena-leak branch Jul 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ cla: yes release notes: yes Include this PR description in the next release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

(C++) memory leak in PackFrom() on arena-allocated Any objects
3 participants