[SERVER-62355] Add mapping from collection namespace to Op Observer Created: 04/Jan/22  Updated: 06/Dec/22  Resolved: 07/Feb/22

Status: Closed
Project: Core Server
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Task Priority: Major - P3
Reporter: Didier Nadeau Assignee: [DO NOT USE] Backlog - Server Serverless (Inactive)
Resolution: Won't Fix Votes: 0
Labels: None
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified

Assigned Teams:
Serverless
Sprint: Server Serverless 2022-01-10
Participants:

 Description   

When an operation occurs, the code go through all the OpObserver and invokes them so they can apply some added logic if needed.

As multiple OpObservers simply check if the collection is equal to a constant value. Creating a type of observer that's bound to a specific collection would allow usage of a map instead of iterating through all observers.

A new mechanism to bind such an observer could be something similar to :

    void addObserver(std::string CollectionNamespace, std::unique_ptr<OpObserver> observer)

This would allow usage of a map to avoid iterating though all the observers.



 Comments   
Comment by Didier Nadeau [ 07/Feb/22 ]

As it turns out the performance issue was not with the vector itself, but rather with the observer (using a map "fixed" the issue by avoiding calls to the observer's code). This issue will be fixed within the observer's code itself instead.

Currently there is likely no benefit to using a map as only 2 observers are bound to a single namespace (tmrs' observer was recently modified and is now watching multiple namespaces). With so few observers and no plan to add anymore at the moment, we will not use an observer map.

Comment by Louis Williams [ 04/Feb/22 ]

Thanks, didier.nadeau! Since it appears as though you have a working solution, I'm going to assign this to Server Serverless to schedule and investigate further. Please add me as a reviewer.

Comment by Didier Nadeau [ 03/Feb/22 ]

Hi louis.williams, I am reopening this ticket as performance BF-24189 was raised following SERVER-61772. I have tried a test implementation of the observer map and it does seem to improve performance : https://jira.mongodb.org/browse/BF-24189?focusedCommentId=4334710&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-4334710. I am running a few more tests and will comment again on this.

Comment by Louis Williams [ 24/Jan/22 ]

Hi didier.nadeau, while I agree that the proposed API makes sense, the performance motivation, which I understand is the reason for filing this ticket, doesn't seem strong enough to warrant such a change.

My belief is that unless the performance impact can be shown, this is not a performance problem until proven otherwise. OpObservers can show up in perf, but not because of the string comparisons. They're expensive because they perform extra writes, acquire locks, etc. But memcmp() is an extremely efficient operation, often optimized down to the processor level.

The number of OpObservers is low enough (I counted 9 on a shard server), that I don't see the map solution as a clear win over the current implementation. The cost of maintaining a map from every namespace to a set of OpObservers seems pretty high. I can think of a simple implementation that registers the same set of OpObservers for every single namespace. This has obvious problems such as space and code complexity. If the map uses hashing, it might be more costly than a few small string comparisons. If the map uses a tree, then we would have chase pointers, which is also costly and cache-inefficient.

To iterate on this solution, we could maintain a set of "common" observers (what we have today) in addition to a map for namespace-specific observers. But I see this as having a similar problem: each OpObserver now has to perform a map lookup in addition to iterating through the common observers.

If you disagree, and this does in fact show up significantly in perf, please feel free to reopen. Additionally, if we see a significant increase in the usage of OpObservers for specific namespaces, perhaps we can reconsider this change. 

Generated at Thu Feb 08 05:54:54 UTC 2024 using Jira 9.7.1#970001-sha1:2222b88b221c4928ef0de3161136cc90c8356a66.