Skip to content

Conversation

@frairon
Copy link
Contributor

@frairon frairon commented Jan 1, 2024

Work in Progress

This is a draft for evaluating the efficiency and consequences of adding (memory) pooling to the goka API.

How it works

When processing messages with goka Processors, messages are decoded using a Codec in every callback call, as well as for every Join or Lookup. The same happens on every View.Get. When using more complex codecs like protobuf, those allocations add up and create pressure in the garbage collector. We would like to try reusing the messages. Protobuf supports Reseting the message upon unmarshalling, so they're a god fit for pooling.
Therefore we need to be able to tell the Codec when the message processing is done and the reference can be reused.

This PR adds a new decode-function called DecodeP which returns an io.Closer interface that must be called when the message can be reused.
Additionally, it also adds a GetP method to the storage.Storage interface to support pooling on disk-level, especially for Views.

Copy link

@paulhenke paulhenke left a comment

Choose a reason for hiding this comment

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

Therefore we need to be able to tell the Codec when the message processing is done and the reference can be reused.

So codecs become stateful? Or were there already before?

Comment on lines 7 to 11
type nullCloser struct{}

func (n *nullCloser) Close() error { return nil }

var NoopCloser = new(nullCloser)

Choose a reason for hiding this comment

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

What is this for, if all the code calling DecodeP is doing a nil check on the io.Closer return.

@frairon
Copy link
Contributor Author

frairon commented Jan 7, 2024

Thanks @paulhenke for all your comments. As said in some comments, the idea of this PR is to draft how pooling could be supported, although goka does not actually use it, only the clients. The codecs would become stateful in some sense, that's true, but they haven't been before and don't have to be. But breaking the API for this is probably a bad idea, so we'll design it in a way that both ways are supported.
But again, we're just in testing/drafting phase now and want to see if implementing all that actually brings some performance gains. If not, the whole thing is dropped right away. So the styling improvements will follow later.

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