Optimize the performance by passing MessageID implementations by pointers

[BewareMyPower]

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.

[BewareMyPower]

Wait for #970 to be merged first.

[tisonkun]

Generally looks good.

Do you have a bench result on this change? Since you declaim that it's a performance improvement.

Also, why do you remove impl_message_bench_test.go?

[shibd]

Also, why do you remove impl_message_bench_test.go?

This test has no assertions and should not be in unit tests.

[tisonkun]

This test has no assertions and should not be in unit tests.

I think they're go benches which will be only triggered if you run go test -bench

[BewareMyPower]

Do you have a bench result on this change? Since you declaim that it's a performance improvement.

I will paste a result soon. Though it might not be significant.

Also, why do you remove impl_message_bench_test.go?

See my PR description.

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.

[BewareMyPower]

Test process

I created a branch from master and apply the following patch to disable ACK grouping (so that each MessageID could be passed)

diff --git a/perf/perf-consumer.go b/perf/perf-consumer.go
index 6b6e411..c7648e3 100644
--- a/perf/perf-consumer.go
+++ b/perf/perf-consumer.go
@@ -79,6 +79,7 @@ func consume(consumeArgs *ConsumeArgs, stop <-chan struct{}) {
                Topic:                          consumeArgs.Topic,
                SubscriptionName:               consumeArgs.SubscriptionName,
                EnableBatchIndexAcknowledgment: consumeArgs.EnableBatchIndexAck,
+               AckGroupingOptions:             &pulsar.AckGroupingOptions{MaxSize: 0},
        })

        if err != nil {

Then, build the perf tool.

cd perf
go build

Since this patch mainly affects the MessageID related operations, let's only test the consumer with each message acknowledged.

Start Pulsar 2.11 standalone. Create two subscriptions sub1 and sub2 on the same topic:

./perf consume --enable-batch-index-ack my-topic -s sub1
./perf consume --enable-batch-index-ack my-topic -s sub2

Then, produce some messages and wait for a while and stop the producer.

./perf produce --batching-num-messages=1 --rate=200000 my-topic

Then we can test the catch-up read. First, test without this patch:

go build
./perf consume --enable-batch-index-ack my-topic -s sub1

Then, apply this patch and repeat the steps above:

git merge bewaremypower/msg-id-pointer
go build
# NOTE: sub1 and sub2 have the same initial position
./perf consume --enable-batch-index-ack my-topic -s sub2

Before running the perf tool each time, restart the standalone.

Test result

Without this patch:

INFO[22:39:09.321] Stats - Consume rate: 8350.1 msg/s -   65.2 Mbps
INFO[22:39:19.321] Stats - Consume rate: 9100.0 msg/s -   71.1 Mbps
INFO[22:39:29.321] Stats - Consume rate: 8750.0 msg/s -   68.4 Mbps
INFO[22:39:39.321] Stats - Consume rate: 8870.0 msg/s -   69.3 Mbps
INFO[22:39:49.321] Stats - Consume rate: 9200.0 msg/s -   71.9 Mbps
INFO[22:39:59.321] Stats - Consume rate: 9020.0 msg/s -   70.5 Mbps
INFO[22:40:09.320] Stats - Consume rate: 8659.9 msg/s -   67.7 Mbps
INFO[22:40:19.321] Stats - Consume rate: 8810.1 msg/s -   68.8 Mbps
INFO[22:40:29.321] Stats - Consume rate: 8960.0 msg/s -   70.0 Mbps
INFO[22:40:39.321] Stats - Consume rate: 8550.0 msg/s -   66.8 Mbps
INFO[22:40:49.321] Stats - Consume rate: 8300.0 msg/s -   64.8 Mbps
INFO[22:40:59.322] Stats - Consume rate: 8420.0 msg/s -   65.8 Mbps

With this patch:

INFO[22:42:04.326] Stats - Consume rate: 8340.1 msg/s -   65.2 Mbps
INFO[22:42:14.327] Stats - Consume rate: 9930.0 msg/s -   77.6 Mbps
INFO[22:42:24.327] Stats - Consume rate: 8970.0 msg/s -   70.1 Mbps
INFO[22:42:34.326] Stats - Consume rate: 9410.0 msg/s -   73.5 Mbps
INFO[22:42:44.326] Stats - Consume rate: 9400.0 msg/s -   73.4 Mbps
INFO[22:42:54.326] Stats - Consume rate: 9260.0 msg/s -   72.3 Mbps
INFO[22:43:04.327] Stats - Consume rate: 9362.3 msg/s -   73.1 Mbps
INFO[22:43:14.326] Stats - Consume rate: 9317.7 msg/s -   72.8 Mbps
INFO[22:43:24.326] Stats - Consume rate: 9100.0 msg/s -   71.1 Mbps
INFO[22:43:34.326] Stats - Consume rate: 8900.0 msg/s -   69.5 Mbps
INFO[22:43:44.326] Stats - Consume rate: 8830.0 msg/s -   69.0 Mbps
INFO[22:43:54.327] Stats - Consume rate: 9180.0 msg/s -   71.7 Mbps

After this patch, the throughput is more stable and is a little higher than before.

@tisonkun

[tisonkun]

Thanks for your report.

Although I think the performance is not quite optimized, checking mid, ok looks weird to me.

I'm +1 as it simplifies code without perf regression.