Performance[MQBSTAT]: lookup for per-appId metrics O(n) -> O(1) #389

678098 · 2024-08-05T21:40:31Z

Previously, we used a bsl::list<ManagedPtr<mwcst::StatContext> > for 2 roles:

Store managed pointers to subcontexts associated with appIds
Lookup to subcontexts for each specific appId

The problem is that the list assumes linear search, which might be too slow if we have a decent number of appIds.

However, due to the interfaces of ManagedPtr (on Solaris), we cannot just use a container which might reallocate its elements on resize. So instead we keep&update 2 collections together: bsl::list to hold ManagedPointers and bsl::unordered_map for fast lookup to raw pointers to the needed StatContexts

Full list of changes:

mqbstat::QueueStatsDomain: hold managed pointers in bsl::list<StatSubContextMp> d_subContextsHolder, lookup to subcontexts with bsl::unordered_map<bsl::string, mwcst::StatContext*> d_subContextsLookup
mqbstat::QueueStatsDomain: pass allocator in constructor
mqbstat::QueueStatsDomain: improve logging when no subcontext found
mqbc::ClusterDataIdentity: add safety check to prevent possible nullptr dereference
mqbmock::Cluster: add safety checks to find cluster misconfiguration early
mqbstat_queuestats.t.cpp: add UT to check per-appId subcontexts and metrics

Signed-off-by: Evgeny Malygin <[email protected]>

678098 · 2024-08-06T02:26:28Z

src/groups/mqb/mqbc/mqbc_clusterdata.cpp

@@ -53,6 +53,8 @@ mqbc::ClusterDataIdentity clusterIdentity(const bslstl::StringRef& name,
    // Create client identity
    bmqp_ctrlmsg::ClientIdentity identity(allocator);
    if (!isRemote) {
+        BSLS_ASSERT_SAFE(netCluster->selfNode());


On the line 65 below, netCluster->selfNode() is dereferenced without checks, which might lead to segfault. This might be achieved by using mock domain/cluster objects.

678098 · 2024-08-06T02:27:24Z

src/groups/mqb/mqbmock/mqbmock_cluster.cpp

@@ -156,6 +156,10 @@ void Cluster::_initializeNetcluster()
                                      : k_LEADER_NODE_ID + 1;
    dynamic_cast<mqbnet::MockCluster*>(d_netCluster_mp.get())
        ->_setSelfNodeId(selfNodeId);
+
+    if (d_isClusterMember) {
+        BSLS_ASSERT_OPT(0 != d_netCluster_mp->selfNode());


Another early safety check for the same problem with dereferencing selfNode with mocks.

678098 · 2024-08-06T02:29:49Z

src/groups/mqb/mqbmock/mqbmock_cluster.cpp

@@ -231,7 +235,12 @@ Cluster::Cluster(bdlbb::BlobBufferFactory* bufferFactory,
 , d_processor()
 {
    // PRECONDITIONS
-    BSLS_ASSERT_OPT(isClusterMember || !isLeader);
+    if (isClusterMember) {
+        BSLS_ASSERT_OPT(!clusterNodeDefs.empty());


Early safety check. Providing an empty clusterNodeDefs makes it certain that selfNode will be nullptr.

678098 · 2024-08-06T02:31:09Z

src/groups/mqb/mqbstat/mqbstat_queuestats.cpp

        BALL_LOGTHROTTLE_WARN(k_MAX_INSTANT_MESSAGES, k_NS_PER_MESSAGE)
-            << "[THROTTLED] No built sub contexts";
+            << "[THROTTLED] No built sub contexts for domain: "
+            << d_statContext_mp->name() << ", appId: " << appId;


New log line looks like
06AUG2024_02:21:59.442 10822:8281000960 WARN /blazingmq/src/groups/mqb/mqbstat/mqbstat_queuestats.cpp:602 MQBSTAT.QUEUESTATS [THROTTLED] No matching StatContext for domain: bmq://mock-domain/abc, appId: bar

Signed-off-by: Evgeny Malygin <[email protected]>

kaikulimu · 2024-08-09T19:23:32Z

However, due to the interfaces of ManagedPtr (on Solaris), we cannot just use a container which might reallocate its elements on resize.

Do you have source to this statement? It is quite inconvenient to have to use two data structures.

kaikulimu

Minor comments

src/groups/mqb/mqbstat/mqbstat_queuestats.cpp

src/groups/mqb/mqbstat/mqbstat_queuestats.t.cpp

Signed-off-by: Evgeny Malygin <[email protected]>

678098 · 2024-08-12T19:50:11Z

Do you have source to this statement? It is quite inconvenient to have to use two data structures.

@kaikulimu ManagedPtr docs
https://bloomberg.github.io/bde-resources/doxygen/bde_api_prod/bslma__managedptr_8h_source.html

However, like 'bsl::auto_ptr' it has unusual "copy-semantics" that transfer ownership of the managed object, rather than making a copy. It should be noted that this signature does not satisfy the requirements for an element-type stored in any of the standard library containers.

So ManagedPtr doesn't satisfy the CopyConstructible requirements https://en.cppreference.com/w/cpp/named_req/CopyConstructible

On SunOS, trying to build a bsl::unordered_map<bsl::string, bslma::ManagedPtr<...> > will cause compilation errors, so as with other container types which rely on reallocating its elements.

2024-08-07 13:57:33 | "/opt/bb/include/bslstl_pair.h", line 2528: Error: Could not find a match for BloombergLP::bslma::ManagedPtr<BloombergLP::mwcst::StatContext>::ManagedPtr<MANAGED_TYPE>(const BloombergLP::bslma::ManagedPtr<BloombergLP::mwcst::StatContext>) needed in void bsl::Pair_Second<BloombergLP::bslma::ManagedPtr<BloombergLP::mwcst::StatContext>>::Pair_Second<BloombergLP::bslma::ManagedPtr<BloombergLP::mwcst::StatContext>>(const BloombergLP::bslma::ManagedPtr<BloombergLP::mwcst::StatContext>&, BloombergLP::bslma::Allocator*, bsl::integral_constant<int, 0>).
2024-08-07 13:57:33 | "/opt/bb/include/bslstl_pair.h", line 2792: Where: While instantiating "void bsl::Pair_Second<BloombergLP::bslma::ManagedPtr<BloombergLP::mwcst::StatContext>>::Pair_Second<BloombergLP::bslma::ManagedPtr<BloombergLP::mwcst::StatContext>>(const BloombergLP::bslma::ManagedPtr<BloombergLP::mwcst::StatContext>&, BloombergLP::bslma::Allocator*, bsl::integral_constant<int, 0>)".
2024-08-07 13:57:33 | "/opt/bb/include/bslstl_pair.h", line 2792: Where: Instantiated from bsl::pair<const bsl::basic_string<char, std::char_traits<char>, bsl::allocator<char>>, BloombergLP::bslma::ManagedPtr<BloombergLP::mwcst::StatContext>>::pair(const bsl::basic_string<char, std::char_traits<char>, bsl::allocator<char>>&, const BloombergLP::bslma::ManagedPtr<BloombergLP::mwcst::StatContext>&, BloombergLP::bslma::Allocator*).
2024-08-07 13:57:33 | "/opt/bb/include/bslma_constructionutil_cpp03.h", line 9370: Where: Instantiated from static void bsl::allocator_traits<bsl::allocator<long double>>::construct<bsl::pair<const bsl::basic_string<char, std::char_traits<char>, bsl::allocator<char>>,

The first message could be simplified:
Error: Could not find a match for ManagedPtr<StatContext>::ManagedPtr<MANAGED_TYPE>(const ManagedPtr<StatContext>)

If you check the ManagedPtr interfaces, there are no constructors accepting other ManagedPtrs qualified as const
https://bloomberg.github.io/bde-resources/doxygen/bde_api_prod/classbslma_1_1ManagedPtr.html

kaikulimu

lgtm

…mberg#389) Signed-off-by: Evgeny Malygin <[email protected]>

678098 requested a review from a team as a code owner August 5, 2024 21:40

678098 added 3 commits August 6, 2024 03:23

Performance[MQBSTAT]: lookup for per-appId metrics O(n) -> O(1)

945bbc2

Signed-off-by: Evgeny Malygin <[email protected]>

UT[MQB]: fix test

ff282de

Signed-off-by: Evgeny Malygin <[email protected]>

MQB: fix preconditions in mqbc_clusterdata, mqbmock_cluster

98af0da

Signed-off-by: Evgeny Malygin <[email protected]>

678098 force-pushed the 240803_rm_list_search branch from 2a9f237 to 98af0da Compare August 6, 2024 02:23

678098 commented Aug 6, 2024

View reviewed changes

Update mqbstat_queuestats.h

588a2c4

Signed-off-by: Evgeny Malygin <[email protected]>

678098 mentioned this pull request Aug 7, 2024

Feat[MQB, MWC]: report the number of tcp connections #384

Merged

678098 requested a review from kaikulimu August 8, 2024 13:14

678098 assigned kaikulimu Aug 8, 2024

kaikulimu reviewed Aug 9, 2024

View reviewed changes

src/groups/mqb/mqbstat/mqbstat_queuestats.cpp Outdated Show resolved Hide resolved

src/groups/mqb/mqbstat/mqbstat_queuestats.t.cpp Show resolved Hide resolved

Review fixes

f13b4cb

Signed-off-by: Evgeny Malygin <[email protected]>

678098 force-pushed the 240803_rm_list_search branch from 7eb93fc to f13b4cb Compare August 12, 2024 17:13

kaikulimu approved these changes Aug 12, 2024

View reviewed changes

678098 merged commit a6a6cef into bloomberg:main Aug 13, 2024
29 checks passed

678098 deleted the 240803_rm_list_search branch August 13, 2024 00:29

alexander-e1off pushed a commit to alexander-e1off/blazingmq that referenced this pull request Oct 24, 2024

Performance[MQBSTAT]: lookup for per-appId metrics O(n) -> O(1) (bloo…

06d1bfd

…mberg#389) Signed-off-by: Evgeny Malygin <[email protected]>

alexander-e1off pushed a commit to alexander-e1off/blazingmq that referenced this pull request Oct 24, 2024

Performance[MQBSTAT]: lookup for per-appId metrics O(n) -> O(1) (bloo…

189a784

…mberg#389) Signed-off-by: Evgeny Malygin <[email protected]>

alexander-e1off pushed a commit to alexander-e1off/blazingmq that referenced this pull request Oct 24, 2024

Performance[MQBSTAT]: lookup for per-appId metrics O(n) -> O(1) (bloo…

0bfcda2

…mberg#389) Signed-off-by: Evgeny Malygin <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Performance[MQBSTAT]: lookup for per-appId metrics O(n) -> O(1) #389

Performance[MQBSTAT]: lookup for per-appId metrics O(n) -> O(1) #389

678098 commented Aug 5, 2024 •

edited

Loading

678098 Aug 6, 2024

678098 Aug 6, 2024

678098 Aug 6, 2024 •

edited

Loading

678098 Aug 6, 2024 •

edited

Loading

kaikulimu commented Aug 9, 2024

kaikulimu left a comment

678098 commented Aug 12, 2024

kaikulimu left a comment

Performance[MQBSTAT]: lookup for per-appId metrics O(n) -> O(1) #389

Performance[MQBSTAT]: lookup for per-appId metrics O(n) -> O(1) #389

Conversation

678098 commented Aug 5, 2024 • edited Loading

678098 Aug 6, 2024

Choose a reason for hiding this comment

678098 Aug 6, 2024

Choose a reason for hiding this comment

678098 Aug 6, 2024 • edited Loading

Choose a reason for hiding this comment

678098 Aug 6, 2024 • edited Loading

Choose a reason for hiding this comment

kaikulimu commented Aug 9, 2024

kaikulimu left a comment

Choose a reason for hiding this comment

678098 commented Aug 12, 2024

kaikulimu left a comment

Choose a reason for hiding this comment

678098 commented Aug 5, 2024 •

edited

Loading

678098 Aug 6, 2024 •

edited

Loading

678098 Aug 6, 2024 •

edited

Loading