-
Notifications
You must be signed in to change notification settings - Fork 336
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
Different MessageID implementations for message Production and Consumption #324
Merged
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
Thanks @dferstay work for this. When we first added other fields to the messageID structure, I had the same considerations. This is the result of our final discussion. Reference to #82 @cckellogg What do you think of it? |
…ption This change splits the `MessageID` implementation in two: 1. `messageID` - A 24 byte structure that contains message identification information only; to be used during message production 2. `trackingMessageID` - A 72 byte structucture that shares the same message identification information as `messageID` and adds `ackTracker`, `acker`, and `receivedTime` fields; to be used during message consumption Micro benchmarks show that passing arguments by value that are less-than four words of memory are optimized by the Go runtime. Results from the pulsar/impl_message_bench_test.go module are below. ``` name time/op ProducerCall 1.46ns ± 5% ProducerCall-4 1.47ns ± 5% ConsumerCall 7.62ns ± 1% ConsumerCall-4 7.53ns ± 5% ```
dferstay
force-pushed
the
split-message-id-impl
branch
from
July 14, 2020 02:21
e8409ef
to
593d7c1
Compare
merlimat
approved these changes
Jul 24, 2020
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 We saw the actual difference with this approach.
BewareMyPower
added a commit
to BewareMyPower/pulsar-client-go
that referenced
this pull request
Feb 27, 2023
### Motivation Currently there are three implementations of the `MessageID` interface: - `messageID`: 24 bytes - `trackingMessageID`: 64 bytes - `chunkMessageID`: 80 bytes However, for all methods of them, the receiver is a value rather than a pointer. It's inefficient because each time a method is called, the copy would happen. Reference: https://go.dev/tour/methods/8 ### Modifications - Change the receiver from value to pointer for all `MessageID` implementations. - Use pointers as the returned values and function parameters for these implementations everywhere. The `trackingMessageID.Undefined` method is removed because it's never used now. Though it's a public method, the struct and its factory function are not exposed, so I think it's reasonable. Remove the benchmark added in apache#324. The result is obvious and this test is meaningless. I tried passing the `trackingMessageID` by pointer and the result reduced from 8.548 ns/op to 1.628 ns/op. It's obvious because a pointer is only 8 bytes while a `trackingMessageID` is 64 bytes. The overhead of accessing by pointers is far less than copying the extra bytes.
BewareMyPower
added a commit
to BewareMyPower/pulsar-client-go
that referenced
this pull request
Feb 27, 2023
### Motivation Currently there are three implementations of the `MessageID` interface: - `messageID`: 24 bytes - `trackingMessageID`: 64 bytes - `chunkMessageID`: 80 bytes However, for all methods of them, the receiver is a value rather than a pointer. It's inefficient because each time a method is called, the copy would happen. Reference: https://go.dev/tour/methods/8 ### Modifications - Change the receiver from value to pointer for all `MessageID` implementations. - Use pointers as the returned values and function parameters for these implementations everywhere. The `trackingMessageID.Undefined` method is removed because it's never used now. Though it's a public method, the struct and its factory function are not exposed, so I think it's reasonable. Remove the benchmark added in apache#324. The result is obvious and this test is meaningless. I tried passing the `trackingMessageID` by pointer and the result reduced from 8.548 ns/op to 1.628 ns/op. It's obvious because a pointer is only 8 bytes while a `trackingMessageID` is 64 bytes. The overhead of accessing by pointers is far less than copying the extra bytes.
BewareMyPower
added a commit
to BewareMyPower/pulsar-client-go
that referenced
this pull request
Feb 27, 2023
### Motivation Currently there are three implementations of the `MessageID` interface: - `messageID`: 24 bytes - `trackingMessageID`: 64 bytes - `chunkMessageID`: 80 bytes However, for all methods of them, the receiver is a value rather than a pointer. It's inefficient because each time a method is called, the copy would happen. Reference: https://go.dev/tour/methods/8 ### Modifications - Change the receiver from value to pointer for all `MessageID` implementations. - Use pointers as the returned values and function parameters for these implementations everywhere. The `trackingMessageID.Undefined` method is removed because it's never used now. Though it's a public method, the struct and its factory function are not exposed, so I think it's reasonable. Remove the benchmark added in apache#324. The result is obvious and this test is meaningless. I tried passing the `trackingMessageID` by pointer and the result reduced from 8.548 ns/op to 1.628 ns/op. It's obvious because a pointer is only 8 bytes while a `trackingMessageID` is 64 bytes. The overhead of accessing by pointers is far less than copying the extra bytes.
BewareMyPower
added a commit
to BewareMyPower/pulsar-client-go
that referenced
this pull request
Feb 28, 2023
### Motivation Currently there are three implementations of the `MessageID` interface: - `messageID`: 24 bytes - `trackingMessageID`: 64 bytes - `chunkMessageID`: 80 bytes However, for all methods of them, the receiver is a value rather than a pointer. It's inefficient because each time a method is called, the copy would happen. Reference: https://go.dev/tour/methods/8 ### Modifications - Change the receiver from value to pointer for all `MessageID` implementations. - Use pointers as the returned values and function parameters for these implementations everywhere. The `trackingMessageID.Undefined` method is removed because it's never used now. Though it's a public method, the struct and its factory function are not exposed, so I think it's reasonable. Remove the benchmark added in apache#324. The result is obvious and this test is meaningless. I tried passing the `trackingMessageID` by pointer and the result reduced from 8.548 ns/op to 1.628 ns/op. It's obvious because a pointer is only 8 bytes while a `trackingMessageID` is 64 bytes. The overhead of accessing by pointers is far less than copying the extra bytes.
BewareMyPower
added a commit
to BewareMyPower/pulsar-client-go
that referenced
this pull request
Feb 28, 2023
### Motivation Currently there are three implementations of the `MessageID` interface: - `messageID`: 24 bytes - `trackingMessageID`: 64 bytes - `chunkMessageID`: 80 bytes However, for all methods of them, the receiver is a value rather than a pointer. It's inefficient because each time a method is called, the copy would happen. Reference: https://go.dev/tour/methods/8 ### Modifications - Change the receiver from value to pointer for all `MessageID` implementations. - Use pointers as the returned values and function parameters for these implementations everywhere. The `trackingMessageID.Undefined` method is removed because it's never used now. Though it's a public method, the struct and its factory function are not exposed, so I think it's reasonable. Remove the benchmark added in apache#324. The result is obvious and this test is meaningless. I tried passing the `trackingMessageID` by pointer and the result reduced from 8.548 ns/op to 1.628 ns/op. It's obvious because a pointer is only 8 bytes while a `trackingMessageID` is 64 bytes. The overhead of accessing by pointers is far less than copying the extra bytes.
BewareMyPower
added a commit
to BewareMyPower/pulsar-client-go
that referenced
this pull request
Feb 28, 2023
### Motivation Currently there are three implementations of the `MessageID` interface: - `messageID`: 24 bytes - `trackingMessageID`: 64 bytes - `chunkMessageID`: 80 bytes However, for all methods of them, the receiver is a value rather than a pointer. It's inefficient because each time a method is called, the copy would happen. Reference: https://go.dev/tour/methods/8 ### Modifications - Change the receiver from value to pointer for all `MessageID` implementations. - Use pointers as the returned values and function parameters for these implementations everywhere. The `trackingMessageID.Undefined` method is removed because it's never used now. Though it's a public method, the struct and its factory function are not exposed, so I think it's reasonable. Remove the benchmark added in apache#324. The result is obvious and this test is meaningless. I tried passing the `trackingMessageID` by pointer and the result reduced from 8.548 ns/op to 1.628 ns/op. It's obvious because a pointer is only 8 bytes while a `trackingMessageID` is 64 bytes. The overhead of accessing by pointers is far less than copying the extra bytes.
BewareMyPower
added a commit
to BewareMyPower/pulsar-client-go
that referenced
this pull request
Mar 1, 2023
### Motivation Currently there are three implementations of the `MessageID` interface: - `messageID`: 24 bytes - `trackingMessageID`: 64 bytes - `chunkMessageID`: 80 bytes However, for all methods of them, the receiver is a value rather than a pointer. It's inefficient because each time a method is called, the copy would happen. Reference: https://go.dev/tour/methods/8 ### Modifications - Change the receiver from value to pointer for all `MessageID` implementations. - Use pointers as the returned values and function parameters for these implementations everywhere. The `trackingMessageID.Undefined` method is removed because it's never used now. Though it's a public method, the struct and its factory function are not exposed, so I think it's reasonable. Remove the benchmark added in apache#324. The result is obvious and this test is meaningless. I tried passing the `trackingMessageID` by pointer and the result reduced from 8.548 ns/op to 1.628 ns/op. It's obvious because a pointer is only 8 bytes while a `trackingMessageID` is 64 bytes. The overhead of accessing by pointers is far less than copying the extra bytes.
BewareMyPower
added a commit
that referenced
this pull request
Mar 1, 2023
…#968) ### Motivation Currently there are three implementations of the `MessageID` interface: - `messageID`: 24 bytes - `trackingMessageID`: 64 bytes - `chunkMessageID`: 80 bytes However, for all methods of them, the receiver is a value rather than a pointer. It's inefficient because each time a method is called, the copy would happen. Reference: https://go.dev/tour/methods/8 ### Modifications - Change the receiver from value to pointer for all `MessageID` implementations. - Use pointers as the returned values and function parameters for these implementations everywhere. The `trackingMessageID.Undefined` method is removed because it's never used now. Though it's a public method, the struct and its factory function are not exposed, so I think it's reasonable. Remove the benchmark added in #324. The result is obvious and this test is meaningless. I tried passing the `trackingMessageID` by pointer and the result reduced from 8.548 ns/op to 1.628 ns/op. It's obvious because a pointer is only 8 bytes while a `trackingMessageID` is 64 bytes. The overhead of accessing by pointers is far less than copying the extra bytes.
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 change splits the
MessageID
implementation in two:messageID
- A 24 byte structure that contains message identificationinformation only; to be used during message production
trackingMessageID
- A 72 byte structucture that shares the samemessage identification information as
messageID
and adds
ackTracker
,acker
, andreceivedTime
fields; to be used during message consumption
Micro benchmarks show that passing arguments by value that are less-than
four words of memory are optimized by the Go runtime. Results from the
pulsar/impl_message_bench_test.go
module are below.Motivation
The messageID structure in pulsar-client-go has the following fields:
The above consumes 72 bytes.
Consider the following:
It would be advantageous to split the MessageID implementation into two structures: one used for message production, the other used for message consumption
Modifications
Split the
MessageID
implementation into two structures; one used during message production:One used during message consumption:
Verifying this change
This change is already covered by existing tests, such as:
Does this pull request potentially affect one of the following parts:
Documentation