Performance[MQB]: inline Put, Push, Ack, Confirm #177

dorjesinpo · 2023-12-22T20:39:43Z

Messages that have fixed destination (such as PUSH/ACK/Relay PUT/Relay CONFIRM) do not have to go on Cluster thread - it is a bottleneck.
Instead, messages can go directly to mqbnet::Channel which enqueues them in thread-safe manner.
This needs synchronization based on AtomicGate which allows thread-safe, efficient checking of the cluster status and thread-safe opening/closing and draining.

PUSH and ACK require statistics update which we move from Cluster to Queue. For this reason the opening queue context (OpenQueueConfirmationCookie) conveys statistics from Queue to Cluster

678098 · 2024-01-04T11:07:35Z

Hi @dorjesinpo, looking

src/groups/mqb/mqbnet/mqbnet_channel.h

src/groups/mqb/mqbmock/mqbmock_queuehandle.h

src/groups/mqb/mqbmock/mqbmock_cluster.cpp

src/groups/mqb/mqbi/mqbi_queueengine.h

src/groups/mqb/mqbi/mqbi_queue.h

src/groups/mqb/mqbblp/mqbblp_clusterproxy.h

src/groups/mqb/mqbblp/mqbblp_clusterqueuehelper.cpp

src/groups/mqb/mqbblp/mqbblp_queuehandle.cpp

dorjesinpo

Thank you for reviewing!

src/groups/mqb/mqbnet/mqbnet_channel.h

src/groups/mqb/mqba/mqba_clientsession.cpp

src/groups/mqb/mqbblp/mqbblp_cluster.cpp

dorjesinpo · 2024-01-10T15:49:41Z

src/groups/mqb/mqbblp/mqbblp_clusterproxy.cpp

        return;  // RETURN
    }

+    d_activeNode_p = activeNode;


Because d_activeNode_p is (half) protected by d_gateActiveNode - it is safe to use if mqbc::GateKeeper::Status(d_gateActiveNode).isOpen()
d_activeNodeManager.activeNode() does not provide such guarantee.
I agree, this calls for refactoring, not in this PR, perhaps

src/groups/mqb/mqbblp/mqbblp_queuehandle.cpp

src/groups/mqb/mqbc/mqbc_clustermembership.cpp

src/groups/mqb/mqbc/mqbc_clusternodesession.h

src/groups/mqb/mqbc/mqbc_clusterstate.cpp

src/groups/mqb/mqbi/mqbi_queue.h

678098 · 2024-01-10T21:19:58Z

@dorjesinpo thank you for fixes, added some comments in discussions and reassigned. You can assign this to @quarter-note later

bmq-oss-ci

Build 190 of commit 7d0e9c7 has completed with FAILURE

pniedzielski · 2024-12-18T22:32:58Z

Assigning back to @dorjesinpo (to put in backlog)

Signed-off-by: Vitaly Dzhitenov <[email protected]>

Signed-off-by: dorjesinpo <[email protected]>

bmq-oss-ci

Build 2613 of commit 615eb54 has completed with FAILURE

678098 · 2025-07-04T13:44:48Z

src/groups/mqb/mqbmock/mqbmock_cluster.cpp

+    BSLS_ANNOTATION_UNUSED int   partitionId,
+    BSLS_ANNOTATION_UNUSED const bmqp::ConfirmMessage& message)


Suggested change

BSLS_ANNOTATION_UNUSED int partitionId,

BSLS_ANNOTATION_UNUSED const bmqp::ConfirmMessage& message)

BSLA_UNUSED int partitionId,

BSLA_UNUSED const bmqp::ConfirmMessage& message)

Recent changes

678098 · 2025-07-04T13:46:31Z

src/groups/mqb/mqbmock/mqbmock_cluster.cpp

+                        appData,
+                        options,
+                        state,
+                        genCount);


I think there will be a segfault if we try to call a function that is not set. At least this is how it was in std c++ implementation.

Do you think it's worth handling empty d_putFunctor for this mock class?

BSLS_ASSERT_SAFE(d_putFunctor);

678098 · 2025-07-04T13:51:56Z

src/groups/mqb/mqbmock/mqbmock_cluster.cpp

 , d_isLeader(isLeader)
 , d_isRestoringState(false)
 , d_processor()
+, d_putFunctor()


Suggested change

, d_putFunctor()

, d_putFunctor(bsl::allocator_arg, d_allocator_p)

Need to handle possibly non-default allocator. This allocator will also be used in operator=

678098 · 2025-07-04T13:54:24Z

src/groups/mqb/mqbmock/mqbmock_cluster.h

+    // Signature of a functor method with one parameter, the processor
+    // handle on which it is being executed.


I think this comment is misleading, since there is more than 1 parameter.
We can remove it or place the updated version in /// block before PutFunctor

678098 · 2025-07-04T14:39:29Z

src/groups/mqb/mqbi/mqbi_queue.h

+    case e_SUCCESS: BSLS_ANNOTATION_FALLTHROUGH;
+    case e_UNAVAILABLE: BSLS_ANNOTATION_FALLTHROUGH;
+    case e_INVALID_PRIMARY: BSLS_ANNOTATION_FALLTHROUGH;
+    case e_INVALID_GEN_COUNT: BSLS_ANNOTATION_FALLTHROUGH;


Suggested change

case e_INVALID_GEN_COUNT: BSLS_ANNOTATION_FALLTHROUGH;

case e_INVALID_GEN_COUNT: BSLA_FALLTHROUGH;

Recent BSLA update

678098 · 2025-07-07T15:15:24Z

src/groups/mqb/mqbblp/mqbblp_cluster.cpp

+    const int id    = message.queueId();
+    const int subId = message.subQueueId();


Suggested change

const int id = message.queueId();

const int subId = message.subQueueId();

Can use directly in 1 place below

678098 · 2025-07-07T15:16:22Z

src/groups/mqb/mqbblp/mqbblp_cluster.cpp

+    int                                 partitionId,
+    const bmqp::PutHeader&              putHeader,
+    const bsl::shared_ptr<bdlbb::Blob>& appData,
+    BSLS_ANNOTATION_UNUSED const bsl::shared_ptr<bdlbb::Blob>& options,


Suggested change

BSLS_ANNOTATION_UNUSED const bsl::shared_ptr<bdlbb::Blob>& options,

BSLA_UNUSED const bsl::shared_ptr<bdlbb::Blob>& options,

678098 · 2025-07-07T15:18:27Z

src/groups/mqb/mqbblp/mqbblp_queuehandle.cpp

 // CREATORS
 QueueHandle::Subscription::Subscription(
-    unsigned int                       subId,
+    unsigned int                       downstreamSubQueueId,


Can subQueueId have a special negative value?

No. There is bmqp::Protocol::k_DEFAULT_SUBSCRIPTION_ID (0)

678098 · 2025-07-07T15:22:16Z

src/groups/mqb/mqbblp/mqbblp_queuehandle.cpp

+            // cache producer stats to avoid lookup
+            d_producerStats.reset();


The comment says about caching, but the code does the opposite and resets a shared_ptr.

678098 · 2025-07-07T15:27:20Z

src/groups/mqb/mqbc/mqbc_clusternodesession.cpp

+
+mqbi::InlineResult::Enum
+ClusterNodeSession::sendAck(const bmqp::AckMessage& ackMessage,
+                            unsigned int            queueId)


Suggested change

unsigned int queueId)

int queueId)

queueId can be negative?

dorjesinpo · 2025-07-16T14:29:02Z

closing in favor of #833

dorjesinpo added the enhancement New feature or request label Dec 22, 2023

dorjesinpo requested a review from a team as a code owner December 22, 2023 20:39

dorjesinpo force-pushed the inline-push-ack branch 3 times, most recently from 6d2f55e to e21c4cd Compare December 26, 2023 01:57

dorjesinpo requested review from 678098 and quarter-note December 26, 2023 12:28

dorjesinpo assigned 678098 Dec 26, 2023

678098 requested changes Jan 8, 2024

View reviewed changes

678098 assigned dorjesinpo and unassigned 678098 Jan 8, 2024

dorjesinpo force-pushed the inline-push-ack branch from e21c4cd to 51bc378 Compare January 10, 2024 17:16

dorjesinpo commented Jan 10, 2024

View reviewed changes

dorjesinpo assigned 678098 and unassigned dorjesinpo Jan 10, 2024

678098 assigned dorjesinpo and unassigned 678098 Jan 10, 2024

dorjesinpo assigned quarter-note and unassigned dorjesinpo Jan 11, 2024

678098 changed the title ~~Inline Put, Push, Ack, Confirm~~ Performance[MQB]: inline Put, Push, Ack, Confirm Aug 6, 2024

bmq-oss-ci bot reviewed Aug 26, 2024

View reviewed changes

pniedzielski assigned dorjesinpo and unassigned quarter-note Dec 18, 2024

pniedzielski removed the request for review from quarter-note December 18, 2024 22:32

dorjesinpo force-pushed the inline-push-ack branch 2 times, most recently from 4becc94 to 36c49a8 Compare March 9, 2025 17:32

dorjesinpo assigned 678098 Mar 11, 2025

dorjesinpo removed their assignment Mar 11, 2025

dorjesinpo added 3 commits April 8, 2025 10:44

Inline Put, Push, Ack, Confirm

1c0db5c

Signed-off-by: Vitaly Dzhitenov <[email protected]>

Updating the docs

8f63216

Signed-off-by: Vitaly Dzhitenov <[email protected]>

new comment

83074ec

Signed-off-by: Vitaly Dzhitenov <[email protected]>

dorjesinpo force-pushed the inline-push-ack branch from 36c49a8 to d7feffd Compare April 8, 2025 14:48

New merge

b3c2c64

Signed-off-by: dorjesinpo <[email protected]>

dorjesinpo force-pushed the inline-push-ack branch from d7feffd to b3c2c64 Compare April 8, 2025 14:51

Merge branch 'main' into inline-push-ack

615eb54

Signed-off-by: dorjesinpo <[email protected]>

bmq-oss-ci bot reviewed May 5, 2025

View reviewed changes

678098 requested changes Jul 7, 2025

View reviewed changes

dorjesinpo mentioned this pull request Jul 15, 2025

Performance[MQB]: Inline Put, Push, Ack, Confirm #833

Open

dorjesinpo closed this Jul 16, 2025

		BSLS_ANNOTATION_UNUSED int partitionId,
		BSLS_ANNOTATION_UNUSED const bmqp::ConfirmMessage& message)

	, d_putFunctor()
	, d_putFunctor(bsl::allocator_arg, d_allocator_p)

		// Signature of a functor method with one parameter, the processor
		// handle on which it is being executed.

	case e_INVALID_GEN_COUNT: BSLS_ANNOTATION_FALLTHROUGH;
	case e_INVALID_GEN_COUNT: BSLA_FALLTHROUGH;

		const int id = message.queueId();
		const int subId = message.subQueueId();

	BSLS_ANNOTATION_UNUSED const bsl::shared_ptr<bdlbb::Blob>& options,
	BSLA_UNUSED const bsl::shared_ptr<bdlbb::Blob>& options,

		// cache producer stats to avoid lookup
		d_producerStats.reset();

Performance[MQB]: inline Put, Push, Ack, Confirm #177

Performance[MQB]: inline Put, Push, Ack, Confirm #177

Uh oh!

Conversation

dorjesinpo commented Dec 22, 2023

Uh oh!

678098 commented Jan 4, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dorjesinpo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

678098 commented Jan 10, 2024

Uh oh!

bmq-oss-ci bot left a comment

Choose a reason for hiding this comment

Uh oh!

pniedzielski commented Dec 18, 2024

Uh oh!

bmq-oss-ci bot left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dorjesinpo commented Jul 16, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants