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.

Sign up for GitHub

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

Jump to bottom

Performance[MQB]: prevent unnecessary allocations in queue engines #390

Merged
merged 10 commits into from
Aug 9, 2024
Merged

Performance[MQB]: prevent unnecessary allocations in queue engines #390

merged 10 commits into from
Aug 9, 2024

Conversation

678098
Copy link
Collaborator

@678098 678098 commented Aug 6, 2024 •
edited
Loading

Drastically reduce the number of memory allocations in RootQueueEngine and RelayQueueEngine

Before:

                                 | Bytes Allocated| -delta-| Max Bytes Allocated| Allocations|  -delta- | Deallocations|  -delta- 
---------------------------------+----------------+--------+--------------------+------------+----------+--------------+----------
        ClusterOrchestrator      |          39,680|        |         241,541,872|  10,294,552|        90|    10,294,409|        90
          *direct*               |           3,248|        |               3,248|          12|          |             1|          
          QueueHelper            |          20,832|        |         241,522,720|  10,293,577|          |    10,293,473|          
            *direct*             |           7,584|        |               8,224|          45|          |            13|          
            Queue                |          13,248|        |         241,515,136|  10,293,532|          |    10,293,460|

After:

                                 | Bytes Allocated|  -delta- | Max Bytes Allocated| Allocations|  -delta-  | Deallocations|  -delta-  
---------------------------------+----------------+----------+--------------------+------------+-----------+--------------+-----------
        ClusterOrchestrator      |      86,295,344|          |          86,295,392|         759|         30|           506|         30
          *direct*               |           3,248|          |               3,248|          12|           |             1|           
          QueueHelper            |      86,275,456|          |          86,275,456|         322|           |           116|           
            *direct*             |           6,480|          |               7,344|          41|           |            19|           
            Queue                |      86,268,976|          |          86,268,976|         281|           |            97|

@678098 678098 requested a review from a team as a code owner August 6, 2024 15:46
@678098 678098 requested a review from dorjesinpo August 6, 2024 16:00
dorjesinpo
dorjesinpo requested changes Aug 8, 2024
View reviewed changes
Copy link
Collaborator

@dorjesinpo dorjesinpo left a comment

Choose a reason for hiding this comment

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

The allocation tables in the description seem to come different scenarios so not much of illustration, but still a good idea.
Why don't we do the same for the RelayQueueEngine?

src/groups/mqb/mqbblp/mqbblp_rootqueueengine.h Outdated
@@ -140,6 +140,9 @@ class RootQueueEngine BSLS_KEYWORD_FINAL : public mqbi::QueueEngine {
// Throttler for when reject messages
// are dumped into temp files.

QueueEngineUtil_AppsDeliveryContext d_context;
Copy link
Collaborator

Choose a reason for hiding this comment

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

just a suggestion, since this is a data member now, a bit more descriptive name to disambiguate from other (future) contexts

@678098 678098 changed the title Performance[MQB]: prevent unnecessary allocations in root queue engine Performance[MQB]: prevent unnecessary allocations in queue engines Aug 8, 2024
@678098 678098 requested a review from dorjesinpo August 8, 2024 23:26
678098
678098 commented Aug 9, 2024
View reviewed changes
src/groups/mqb/mqbblp/mqbblp_rootqueueengine.cpp Outdated
@@ -1238,23 +1239,21 @@ void RootQueueEngine::afterNewMessage(
d_queueState_p->queue()));

// Deliver new messages to active (alive and capable to deliver) consumers
d_appsDeliveryContext.d_doRepeat = true;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Note that we do reset() when we enter the while loop

dorjesinpo
dorjesinpo previously approved these changes Aug 9, 2024
View reviewed changes
Copy link
Collaborator

@dorjesinpo dorjesinpo left a comment

Choose a reason for hiding this comment

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

Just a minor note wrt d_doRepeat = true, a do while loop could look good

@678098
Copy link
Collaborator Author

678098 commented Aug 9, 2024

Just a minor note wrt d_doRepeat = true, a do while loop could look good

Good suggestion, applied

@678098 678098 merged commit bbc4d12 into main Aug 9, 2024
31 checks passed
@678098 678098 deleted the 678098-patch-4 branch August 9, 2024 16:02
alexander-e1off pushed a commit to alexander-e1off/blazingmq that referenced this pull request Oct 24, 2024
@678098 @alexander-e1off
Performance[MQB]: prevent unnecessary allocations in queue engines (b…
9aabde8 
…loomberg#390)


Signed-off-by: Evgeny Malygin <[email protected]>
alexander-e1off pushed a commit to alexander-e1off/blazingmq that referenced this pull request Oct 24, 2024
@678098 @alexander-e1off
Performance[MQB]: prevent unnecessary allocations in queue engines (b…
46e7f22 
…loomberg#390)


Signed-off-by: Evgeny Malygin <[email protected]>
alexander-e1off pushed a commit to alexander-e1off/blazingmq that referenced this pull request Oct 24, 2024
@678098 @alexander-e1off
Performance[MQB]: prevent unnecessary allocations in queue engines (b…
2a4c44a 
…loomberg#390)


Signed-off-by: Evgeny Malygin <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dorjesinpo dorjesinpo dorjesinpo approved these changes

Assignees

@dorjesinpo dorjesinpo

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@678098 @dorjesinpo