[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 :
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 |
| 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. |