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

Enabling arenas in runtime proto #348

Merged
merged 4 commits into from
Apr 22, 2021
Merged

Conversation

mkruskal-google
Copy link
Contributor

This will be enabled by default in future versions, and is necessary to generate protobuf arena methods.

@mkruskal-google mkruskal-google changed the title Enabling arenas in runtime proto. Enabling arenas in runtime proto Apr 20, 2021
Copy link
Member

@antoninbas antoninbas left a comment

Choose a reason for hiding this comment

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

Is there any drawback to enabling arenas unconditionally?

@mkruskal-google
Copy link
Contributor Author

mkruskal-google commented Apr 20, 2021

Looking at when this was flipped on by default back in early 2020, it looks like the main drawback is that the binary size increases a bit. This should be marginal though, and this flag will be deprecated and defaulted to true in future protobuf versions.

Since you aren't using arenas, it basically just adds some dead generated code.

@mkruskal-google
Copy link
Contributor Author

Are there any outstanding issues? I don't seem to have permission to merge

@antoninbas
Copy link
Member

@mkruskal-google, no I was giving other folks a chance to review. I think we can proceed with the merge.

@antoninbas antoninbas merged commit e9c0d19 into p4lang:main Apr 22, 2021
@mkruskal-google
Copy link
Contributor Author

mkruskal-google commented Apr 22, 2021 via email

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

Successfully merging this pull request may close these issues.

3 participants