Skip to content

Conversation

@Anarh2404
Copy link
Contributor

Proposed Changes

This PR based on awesome @stebet work in #706
I made some improvements and achieved even greater reduction in memory consumption and increased performance.
Here is what I did:

  • Reduce the number of async paths (which generates AsyncStateMachine) in pipeline reader
  • Improve binary readers/writers

All tests run, except for the APIApproval

Types of Changes

  • Bug fix (non-breaking change which fixes issue #NNNN)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause an observable behavior change in existing systems)
  • Documentation improvements (corrections, new content, etc)
  • Cosmetic change (whitespace, formatting, etc)

Checklist

  • I have read the CONTRIBUTING.md document
  • I have signed the CA (see https://cla.pivotal.io/sign/rabbitmq)
  • All tests pass locally with my changes
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)
  • Any dependent changes have been merged and published in related repositories

Further Comments

I added netstandard2.1 and few #if derectives, because some perf critical API is only available in the netstandard2.1/netcoreapp3.0. For netstandard2.0 and net461 added fallback to a less optimal implementation.

Memory test

512byte payload
#706
stebet_pr
This PR
my_pr

Simple perfomance test on my home pc

1 publisher, 1 consumer, no ack
Nuget 5.1.2
5 1 2_no_ack
#706
stebet_no_ack
This PR
my_pr_no_ack

1 publisher, 1 consumer, manual ack, 64 prefetch count
Nuget 5.1.2
5 1 2_manual_ack_64_prefetch_count
#706
stebet_manual_ack_64_prefetch_count
This PR
my_pr_manual_ack_64_prefetch_count

Copy link
Contributor

@michaelklishin michaelklishin left a comment

Choose a reason for hiding this comment

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

Thank you. Please remove the test program. Publishing it in a separate repo under your account should be sufficient.

@michaelklishin
Copy link
Contributor

Please rebase this against master.

Out of curiosity, do most of the gains come from the switch to BinaryBufferReader? Or the reduced number of pipeline paths

@lukebakken
Copy link
Collaborator

@Anarh2404 thanks for this pull request.

We can't merge this pull request as-is into master. You should open your pull request against the allocationImprovements branch in the https://github.com/stebet/rabbitmq-dotnet-client.git` fork of this repository.

Or, @stebet - would you mind reviewing these changes and, if acceptable, merge them into your stebet:allocationImprovements fork & branch? Thanks.

@lukebakken lukebakken closed this Feb 24, 2020
@lukebakken lukebakken self-assigned this Feb 24, 2020
@lukebakken lukebakken added this to the 6.0.0 milestone Feb 24, 2020
@lukebakken
Copy link
Collaborator

@stebet - and by "these changes" I mean the changes in @Anarh2404 's commit: 3147ab5

@stebet
Copy link
Contributor

stebet commented Feb 24, 2020

I'll take a look tomorrow :)

@Anarh2404
Copy link
Contributor Author

@michaelklishin BinaryBufferReader has reduced memory consumption, but not only it. Reduction of async paths increased throughput.

@lukebakken @stebet I think it would be nice if this PR would be merged into a stebet:allocationImprovements, of course, if this acceptable.

@Anarh2404
Copy link
Contributor Author

Made a test branch where i merged this PR and #732 :)

test_branch

@Anarh2404
Copy link
Contributor Author

@michaelklishin test program from the #706

@Anarh2404 Anarh2404 mentioned this pull request Mar 9, 2020
11 tasks
@lukebakken lukebakken added the next-gen-todo If a rewrite happens, address this issue. label Mar 30, 2020
@lukebakken lukebakken removed this from the 6.0.0 milestone Mar 30, 2020
@lukebakken
Copy link
Collaborator

I am assuming that the changes in this PR will be investigated for version 7.0.0

@lukebakken lukebakken added this to the 7.0.0 milestone Mar 30, 2020
@lukebakken lukebakken modified the milestones: 8.0.0, 7.0.0 Mar 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

next-gen-todo If a rewrite happens, address this issue.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants