Skip to content

Clean up Message.Reset #489

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.

Sign up for GitHub

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

Jump to bottom
Merged
zenhack merged 3 commits into from
Mar 28, 2023
Merged

Clean up Message.Reset #489

zenhack merged 3 commits into from
Mar 28, 2023

Conversation

@lthibault
Copy link
Collaborator

@lthibault lthibault commented Mar 24, 2023 •
edited
Loading

  • Release Arena when Message.Reset is called
  • Introduce Message.Release as shortcut for Message.Reset(nil)
  • Clean up Message.Reset(nil) call sites
  • Re-use CapTable slice in Message.

@lthibault lthibault requested a review from zenhack March 24, 2023 22:49
@lthibault lthibault self-assigned this Mar 24, 2023
@lthibault lthibault force-pushed the cleanup/message-reset branch from ce07157 to e07929b Compare March 24, 2023 23:39
lthibault
lthibault commented Mar 25, 2023
View reviewed changes
zenhack
zenhack suggested changes Mar 27, 2023
View reviewed changes
Copy link
Contributor

@zenhack zenhack left a comment

Choose a reason for hiding this comment

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

Mostly looks good, a couple issues.

@lthibault
Copy link
Collaborator Author

lthibault commented Mar 28, 2023

@zenhack Comments addressed. As per my comment in Matrix, I felt it was best to turn OutgoingMessage and IncomingMessage into interfaces, so the diff has grown to include their respective call sites. Should be pretty straightforward though.

zenhack
zenhack suggested changes Mar 28, 2023
View reviewed changes
rpc/transport/transport.go
// *capnp.Message will be released. This will also release any clients
// in the message's CapTable and release its Arena.
//
// The Arena in the returned message should be fast at allocating new
Copy link
Contributor

@zenhack zenhack Mar 28, 2023

Choose a reason for hiding this comment

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

Not really related to this PR (no need to address here) but: Is there actually a reason for this should? Afaik there's nothing in the RPC subsystem that would make SingleSegment a problem.

Copy link
Collaborator Author

@lthibault lthibault Mar 28, 2023

Choose a reason for hiding this comment

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

I always assumed it was because the PlaceArgs call might allocate lots of (potentially large) structs? In other words, it would be a case of "we're handing off control the user, so make sure we can handle whatever they do". 🤷‍♂️

@lthibault lthibault requested a review from zenhack March 28, 2023 16:23
@zenhack zenhack merged commit 1a829fd into main Mar 28, 2023
@kwilczynski kwilczynski mentioned this pull request Jul 30, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@zenhack zenhack zenhack approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@lthibault lthibault

Labels

cleanup enhancement

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@lthibault @zenhack