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

[Issue 13285][pulsar-client] Optimize MessageBuilder and SharedBuffer to avoid unnecessary memory copy. #13293

Merged
merged 6 commits into from
Dec 16, 2021

Conversation

tongsucn
Copy link
Contributor

@tongsucn tongsucn commented Dec 14, 2021

Fixes #13285

Motivation

See more in #13285

Modifications

  1. Replaced std::vector<char> with std::string as the internal storage of SharedBuffer.
  2. Added new interface MessageBuilder::setContent(std::string&&).
  3. Added new move-constructors in SharedBuffer.

Does this pull request potentially affect one of the following parts:

  • Dependencies (does it add or upgrade a dependency): no
  • The public API: no
  • The schema: no
  • The default values of configurations: no
  • The wire protocol: no
  • The rest endpoints: no
  • The admin cli options: no
  • Anything that affects deployment: no

Documentation

Check the box below and label this PR (if you have committer privilege).

Need to update docs?

  • no-need-doc

It does not add any new public interface, the comment on the function declaration is enough.

@github-actions
Copy link

@tongsucn:Thanks for your contribution. For this PR, do we need to update docs?
(The PR template contains info about doc, which helps others know more about the changes. Can you provide doc-related info in this and future PR descriptions? Thanks)

@github-actions
Copy link

@tongsucn:Thanks for providing doc info!

@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Dec 14, 2021
@merlimat merlimat added this to the 2.10.0 milestone Dec 14, 2021
Copy link
Contributor

@merlimat merlimat left a comment

Choose a reason for hiding this comment

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

@tongsucn Can you also add a unit tests for the new setContent() method?

@BewareMyPower
Copy link
Contributor

I have a suggestion for testing the memory copy doesn't occur after this change, but it might depend on the implementation of std::string.

TEST(MessageBuilderTest, testSetContent) {
    std::string value;
    value.resize(1024, 'x');
    const void* originalAddress = &value[0];
    {
        auto msg = MessageBuilder().setContent(value).build();
        ASSERT_NE(msg.getData(), originalAddress);
    }
    {
        auto msg = MessageBuilder().setContent(std::move(value)).build();
        ASSERT_EQ(msg.getData(), originalAddress);
    }
}

@tongsucn
Copy link
Contributor Author

tongsucn commented Dec 15, 2021

auto msg = MessageBuilder().setContent(value).build();

@BewareMyPower I think the first setContent assertion should be fine. Because it internally allocates a same-sized std::string and calls std::copy explicitly, rather than using std::string's copy constructor. Thanks bro

@tongsucn
Copy link
Contributor Author

@tongsucn Can you also add a unit tests for the new setContent() method?

@merlimat OK, I'll add some tests today.

@codelipenghui
Copy link
Contributor

@tongsucn Any update on this PR?

@tongsucn
Copy link
Contributor Author

@codelipenghui Just added a test case on MessageBuilder::setContent.

@merlimat
Copy link
Contributor

merlimat commented Dec 16, 2021

@tongsucn You should either rebase or merge with master as there were some merge conflicts that make the checkstyle to fail. It's already fixed in latest master.

@tongsucn
Copy link
Contributor Author

@tongsucn You should either rebase or merge with master as there were some merge conflicts that make the checkstyle to fail. It's already fixed in latest master.

@merlimat done

@merlimat merlimat merged commit 6ddb533 into apache:master Dec 16, 2021
@merlimat merlimat added the type/enhancement The enhancements for the existing features or docs. e.g. reduce memory usage of the delayed messages label Dec 16, 2021
fxbing pushed a commit to fxbing/pulsar that referenced this pull request Dec 19, 2021
… to avoid unnecessary memory copy. (apache#13293)

* [Issue 13285][pulsar-client] Optimize MessageBuilder and SharedBuffer to avoid unnecessary memory copy.

* [Issue 13285][pulsar-client] Fixed an error in move-assigment-operator.

* [Issue 13285][pulsar-client] Added new test case for MessageBuilder::setContent.

* [Issue 13285][pulsar-client] Fixed bug in test case.

* [Issue 13285][pulsar-client] Fixed bug in test case.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc-not-needed Your PR changes do not impact docs type/enhancement The enhancements for the existing features or docs. e.g. reduce memory usage of the delayed messages
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[cpp-client] Optimize MessageBuild and SharedBuffer to avoid unnecessary memory copy.
4 participants