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

Binding multicast implementation #292

Merged
merged 10 commits into from
Jan 29, 2020

Conversation

slinkydeveloper
Copy link
Member

Replaces #282 & #283

Starting from @alanconway's work in #283:

  • Added WithAcksBeforeFinish from Enable a binding.Message to be consumed more times #282
  • From the discussion in WIP: Copying messages for multicast #283, implemented an API to buffer a message. Two methods are provided:
    • CopyMessage to drain the input message without binding the lifecycle (it explicitly require the manual invocation of inputMessage.Finish())
    • BufferMessage to drain the input message binding the lifecycle of the new message with the input message
  • Buffered messages uses a memory pool to store the message payload
  • Reorganized modules: now EventMessage & BindingTransport adapter are under binding/event, buffering methods under binding/buffering & mocks in binding/test
  • Added test for BindingTransport

alanconway and others added 8 commits January 27, 2020 16:31
StructMessage and EventMessage are intended to be full implementations of the
Message interface that are independent of any transport and can be used to hold
a copy of a message in memory independently of the life of an incoming transport
message (e.g. for multicast, queuing etc.) CopyMessage copies an incoming
Message, the copy can be read many times and is independent of the original
message lifecycle (e.g. you might Finish() the original before you're done
multicasting the copy depending on policy)

They are a lot like your MockXMessages but they are not mocks, they're complete
Message implementations. Possibly there's some code overlap that can be cleaned up.

Please review the code carefully. I like what you've done to reduce copying but
there was a lot to take in, so this is a bit rushed.

One question: I intended for 2 types of message representation - Binary and
Structured. Message.Event() and EventMessage were really just implementations of
Binary encoding - not meant to be a third representation. I think you could drop
Message.Event() now that you have ToEvent(Message) so bindings only have to
implement the methods Structured() and Binary().

Signed-off-by: Alan Conway <[email protected]>
Signed-off-by: Francesco Guardiani <[email protected]>
Test integration CopyMessage + WithAcksBeforeFinish

Signed-off-by: Francesco Guardiani <[email protected]>
Added multi senders

Signed-off-by: Francesco Guardiani <[email protected]>
* CopyMessage moved in buffering submodule
* CopyMessage splitted to CopyMessage and BufferMessage (one binds the lifecycle, the other no)
* CopyMessage & BufferMessage implements buffer pooling to decrease memory allocations
* EventMessage, ToEvent & Transport adapter moved in event submodule

Signed-off-by: Francesco Guardiani <[email protected]>
@slinkydeveloper slinkydeveloper changed the title Binding multicast implementation WIP: Binding multicast implementation Jan 27, 2020
@slinkydeveloper
Copy link
Member Author

slinkydeveloper commented Jan 27, 2020

A couple of data:

Comparison with #282 & #283

#282 still remain the fastest, but this PR is faster than #283 and allocates less mostly because of pooling. Some limit cases (focused in particular on high parallelism + high number of senders):

Test case #282 ns/ops #283 ns/ops #292 ns/ops
1Kb, Parallelism 4, 8 senders 25919 27902 43152
4Kb, Parallelism 4, 8 senders 29688 35177 51306
8Kb, Parallelism 4, 8 senders 33379 52616 66403
Test case #282 allocs/ops #283 allocs/ops #292 allocs/ops
1Kb, Parallelism 4, 8 senders 26343 27762 30321
4Kb, Parallelism 4, 8 senders 28371 36994 42620
8Kb, Parallelism 4, 8 senders 29433 49307 59026

All results:

Binary buffered representation

Using cloudevents.Context in buffered binary 62bfa7b:

goos: linux
goarch: amd64
pkg: github.com/cloudevents/sdk-go/pkg/binding/buffering
BenchmarkBufferMessageFromStructured-8           4837137               219 ns/op             144 B/op          4 allocs/op
BenchmarkBufferMessageFromBinary-8                303960              4028 ns/op            1761 B/op         20 allocs/op
PASS
ok      github.com/cloudevents/sdk-go/pkg/binding/buffering     2.607s

Using plain map b766a97:

goos: linux
goarch: amd64
pkg: github.com/cloudevents/sdk-go/pkg/binding/buffering
BenchmarkBufferMessageFromStructured-8           4070000               271 ns/op             144 B/op          4 allocs/op
BenchmarkBufferMessageFromBinary-8               1000000              1186 ns/op             848 B/op          9 allocs/op
PASS
ok      github.com/cloudevents/sdk-go/pkg/binding/buffering     2.628s

@slinkydeveloper slinkydeveloper changed the title WIP: Binding multicast implementation Binding multicast implementation Jan 27, 2020
@n3wscott
Copy link
Member

This all LGTM at quick look, I will take a closer look in a bit.

@n3wscott
Copy link
Member

LGTM

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