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

Avoid copying OpSendMsg when sending messages #308

Merged

Conversation

BewareMyPower
Copy link
Contributor

@BewareMyPower BewareMyPower commented Aug 28, 2023

Fixes #306

Motivation

OpSendMsg is a struct whose size is 400 bytes. We should avoid the copy operation on it.

Modifications

Pass the unique_ptr<OpSendMsg> everywhere instead of OpSendMsg.

  • Use unique_ptr<OpSendMsg> as the element of the pending message queue in ProducerImpl and disable the copy constructor and assignment for OpSendMsg.
  • Add SendArgument, which includes the necessary fields to construct a CommandSend request. Use shared_ptr rather than unique_ptr to store SendArgument in OpSendMsg because the producer might need to resend the message so the SendArgument object could be shared by ProducerImpl and ClientConnection.

This patch is more like a refactor because it is not a key factor that affects the throughput or latency.

Documentation

  • doc-required
    (Your PR needs to update docs and you will update later)

  • doc-not-needed
    (Please explain why)

  • doc
    (Your PR contains doc changes)

  • doc-complete
    (Docs have been already added)

Fixes apache#306

### Motivation

`OpSendMsg` is a struct whose size is 400 bytes. We should avoid the
copy operation on it.

### Modifications

Pass the `unique_ptr<OpSendMsg>` everywhere instead of `OpSendMsg`.
- Use `unique_ptr<OpSendMsg>` as the element of the pending message
  queue in `ProducerImpl` and disable the copy constructor and
  assignment for `OpSendMsg`.
- Add `SendArgument`, which includes the necessary fields to construct a
  `CommandSend` request. Use `shared_ptr` rather than `unique_ptr` to
  store `SendArgument` in `OpSendMsg` because the producer might need to
  resend the message so the `SendArgument` object could be shared by
  `ProducerImpl` and `ClientConnection`.

This patch is more like a refactor because the compiler optimization
might reduce unnecessary copying.
@BewareMyPower BewareMyPower added this to the 3.4.0 milestone Aug 28, 2023
@BewareMyPower BewareMyPower self-assigned this Aug 28, 2023
@BewareMyPower
Copy link
Contributor Author

BewareMyPower commented Aug 28, 2023

Before this patch, ~OpSendMsg could be called in various places (Download the flamegraphs.tar.gz and search .*~OpSendMsg.* in the FlameGraph):

  • Directly in ProducerImpl::ackReceived (4.69%)
  • pendingMessagesQueue_.pop_front() in ProducerImpl::ackReceived (0.47%)
  • Destructor of an Asio callback (std::tuple<std::shared_ptr<pulsar::ClientConnection>, pulsar::OpSendMsg>) (1.56%)
  • BatchMessageContainerBase::processAndClear (0.78%)
  • Directly in ackReceived (another call in event loop) (0.16%)
image

After this patch, ~OpSendMsg could only be called in pendingMessagesQueue_.pop_front() (3.65%)

image

The overhead is the indirect access to the fields of OpSendMsg but it does not make any difference.

Before:

2023-08-28 09:03:36.292 INFO  [139947806231168] PerfProducer:415 | Throughput produced: 79317  msg/s --- 619.664 Mbit/s --- Lat avg: 4.20555 ms -- Lat 99pct: 6.07831 ms
2023-08-28 09:03:56.292 INFO  [139947806231168] PerfProducer:415 | Throughput produced: 78674  msg/s --- 614.641 Mbit/s --- Lat avg: 4.12029 ms -- Lat 99pct: 5.97408 ms
2023-08-28 09:04:16.293 INFO  [139947806231168] PerfProducer:415 | Throughput produced: 78616.9  msg/s --- 614.194 Mbit/s --- Lat avg: 4.10731 ms -- Lat 99pct: 5.85847 ms
2023-08-28 09:04:36.293 INFO  [139947806231168] PerfProducer:415 | Throughput produced: 78646.8  msg/s --- 614.428 Mbit/s --- Lat avg: 4.09198 ms -- Lat 99pct: 5.45791 ms

After:

2023-08-28 09:00:53.307 INFO  [140375521159808] PerfProducer:415 | Throughput produced: 79352.7  msg/s --- 619.943 Mbit/s --- Lat avg: 4.16017 ms -- Lat 99pct: 5.75904 ms
2023-08-28 09:01:13.307 INFO  [140375521159808] PerfProducer:415 | Throughput produced: 78744  msg/s --- 615.188 Mbit/s --- Lat avg: 4.22007 ms -- Lat 99pct: 6.30157 ms
2023-08-28 09:01:33.307 INFO  [140375521159808] PerfProducer:415 | Throughput produced: 78745.8  msg/s --- 615.202 Mbit/s --- Lat avg: 4.17825 ms -- Lat 99pct: 5.72936 ms
2023-08-28 09:01:53.307 INFO  [140375521159808] PerfProducer:415 | Throughput produced: 78649.5  msg/s --- 614.449 Mbit/s --- Lat avg: 4.22108 ms -- Lat 99pct: 6.32692 ms
2023-08-28 09:02:13.309 INFO  [140375521159808] PerfProducer:415 | Throughput produced: 78800.1  msg/s --- 615.626 Mbit/s --- Lat avg: 4.1792 ms -- Lat 99pct: 6.72896 ms

@BewareMyPower BewareMyPower merged commit cac5e1d into apache:main Aug 31, 2023
@BewareMyPower BewareMyPower deleted the bewaremypower/optimize-opsendmsg-copy branch August 31, 2023 13:55
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.

[Performance] Unexpected copy of OpSendMsg
2 participants