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

move block allocation into message queue #140

Merged
merged 1 commit into from
Jan 19, 2021

Conversation

acruikshank
Copy link
Contributor

Motivation

Currently block allocation is split between allocation in the response builder and release in the message queue. This works, but only if the components cooperate, and it is fragile in the long term. The PR brings allocation and release into the message queue.

Proposed Changes

  • Add a context.Context to the BuildMessage function (in all interfaces and implementations)
  • Add the AllocateBlockMemory function to the MessageQueue Allocator interface.
  • Move the delay logic from ResponseAssembler.execute to MessageQueue.AllocateAndBuildMessage and remove the ResponseAssembler Allocator interface.
  • Document the possible block in AllocateAndBuildMessage functions.

Copy link
Collaborator

@hannahhoward hannahhoward left a comment

Choose a reason for hiding this comment

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

LGTM.

I think.

Goodness it's not a good sign that I can't remember for sure if this blocks in the right place. I think it does. As long as the block occurs as needed in the thread that is performing the selector traversal, I think this works.

But to confirm, ultimately, the block runs almost in exactly the same place of execution, just later inside a function call.

@hannahhoward hannahhoward merged commit 39076f4 into master Jan 19, 2021
@aschmahmann aschmahmann mentioned this pull request Feb 18, 2021
73 tasks
@mvdan mvdan deleted the chore/move_allocation_into_message_queue branch December 15, 2021 14:16
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.

2 participants