multi: Make NewMessage() usable for creating messages for reading #591
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This commit changes all occurrences of
NewMessage(NewSingleSegmentArena(nil)) to NewSingleSegmentArena(nil) and
all occurrences of NewMessage(NewMultiSegmentArena(nil)) to
NewMultiSegmentArena(nil).
Also, occurrences of Message{Arena: XXX} (where XXX is either a single
or multi segment arena) are changed when possible as well.
In the future, this will allow protecting Message values from wrong
usage by enforcing the use of NewMessage to initialize message objects
(instead of use of zero valued messages).
matheusd
force-pushed
the
new-message-api
branch
from
9e3a61a to
153d699
Compare
The passed slice cannot be guaranteed to be from the pool, therefore it should not be returned there during Release.
Previously, NewMessage() would always attempt to allocate the first word of the first segment and would consider arenas with more than 1 segment as not usable. This restriction is lifted in this commit by only attempting to allocate the first segment and not the root pointer space. This allows passing a fully populated Arena object to Reset(), which in turns allows passing it to NewMessage. This will be helpful by ensuring NewMessage can be used in all situations where a message is needed (not just for writing). NOTE: this is a breaking API change. Before this commit, NewMessage would fail for read-only arenas or that otherwise could not allocate space for the root pointer. The allocation for the root pointer space is now enforced only when (1) performing an initial allocation or (2) setting the root pointer. Enforcement on (2) is only necessary because one call path in file fileparts.go makes a call to SetRoot before attempting to allocate space in the message. This is likely an erroneous call and should be investigated in the future in order to simplify the code.
This changes all ocurrences of message instantiation to use NewMessage(). This unifies all code for message init under a single code path. In the future, it may be possible to make all message fields unexported in order to better enforce message invariants.
matheusd
force-pushed
the
new-message-api
branch
from
4ad8dde to
dcb3395
Compare
matheusd
changed the title
lthibault
approved these changes
Dec 27, 2024
There was a problem hiding this comment.
Hey @matheusd, my sincerest apologies for letting this sit for so long. I've been dealing with some rather difficult life circumstances the past few months, but I am progressively easing back into things.
These changes LGTM. Can you remind me where we are in your roadmap? Is there any follow-up in the pipeline?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR modifies
Reset()(and consequentlyNewMessage()) so that it can be used to initialize messages for reading.Before this PR,
Reset()would enforce that space for the root pointer was allocated in the associated arena, which could not be done for read-only messages. This PR changes it so that space for the root pointer is only allocated when strictly necessary.In the future, this will allow protecting Message values from wrong usage by enforcing the use of NewMessage to initialize message objects (instead of use of zero valued messages).
Warning
This is a breaking API change.
NewMessage()would fail if the underlying arena could not allocate space, and if this PR is merged that will not be true anymore.