sysrepo icon indicating copy to clipboard operation
sysrepo copied to clipboard

subscription filtering optimization

Open irfanHaslanded opened this issue 3 years ago • 7 comments

If there is a change made to a module, events are written to all subscribers and the xpath filtering of the event happens after the listen thread is woken up during sr_shmsub_change_listen_process_module_events

Is it possible to do this filtering earlier at the time of sr_shmsub_change_notify_next_subscription?

Is there any reason why we are doing the filtering later in the listen thread?

Please let me know your thoughts on this.

#0  sr_shmsub_change_listen_filter_is_valid (sub=0x555555612c00, diff=0x7ffff00026d0) at /home/irfan/opensource/sysrepo/src/shm_sub.c:2215
#1  0x00007ffff7f76fcc in sr_shmsub_change_listen_process_module_events (change_subs=0x5555555616c0, conn=0x555555559940)
    at /home/irfan/opensource/sysrepo/src/shm_sub.c:2576
#2  0x00007ffff7f3a510 in sr_subscription_process_events (subscription=0x555555564ea0, session=0x0, stop_time_in=0x7ffff7b2dc50)
    at /home/irfan/opensource/sysrepo/src/sysrepo.c:4081
#3  0x00007ffff7f79a67 in sr_shmsub_listen_thread (arg=0x555555564ea0) at /home/irfan/opensource/sysrepo/src/shm_sub.c:3650
#0  sr_shmsub_change_notify_next_subscription (conn=0x555555559940, mod=0x5555555dfd30, ds=SR_DS_RUNNING, ev=SR_SUB_EV_CHANGE, last_priority=0,
    next_priority_p=0x7fffffffd77c, sub_count_p=0x7fffffffd778, opts_p=0x7fffffffd71c) at /home/irfan/opensource/sysrepo/src/shm_sub.c:744
#1  0x00007ffff7f738b5 in sr_shmsub_change_notify_change (mod_info=0x7fffffffd860, orig_name=0x0, orig_data=0x0, timeout_ms=5000,
    cb_err_info=0x7fffffffd898) at /home/irfan/opensource/sysrepo/src/shm_sub.c:1230
#2  0x00007ffff7f38988 in sr_changes_notify_store (mod_info=0x7fffffffd860, session=0x555555574330, timeout_ms=5000, cb_err_info=0x7fffffffd898)
    at /home/irfan/opensource/sysrepo/src/sysrepo.c:3390
#3  0x00007ffff7f38d34 in sr_apply_changes (session=0x555555574330, timeout_ms=5000) at /home/irfan/opensource/sysrepo/src/sysrepo.c:3489

irfanHaslanded avatar Feb 08 '22 10:02 irfanHaslanded

Yes, there is. A single SHM segment is used to communicate all changes in a YANG module. But there could be lots of subscribers to changes of this particular YANG module so the originator writes them all into the SHM and it is up to the subscribers to each filter only the changes they are interested in. There is another reason being that when you are getting data in CHANGE callbacks, all the changes of the module are first applied and the data provided with them, irrespective of what filter you have used for the changes (meaning filter works only for the change iterator, not getting modified data). I was pestered quite a lot to support this latter behavior by people moving from sysrepo v0.7.x.

michalvasko avatar Feb 08 '22 11:02 michalvasko

I understand this reasoning. However, in our use case, this presents a scalability problem. We have subscriptions that only need to be notified for the xpath, and not interested in other changes, for example ietf-interfaces, where an app cares only about a single interface.

I request to have an enhancement, with an option, SR_SUBSCR_PRODUCER_FILTERING at the time of subscription, to enable this behavior. I can make a pull request if this seems okay, or please advise any alternate ideas.

irfanHaslanded avatar Feb 16 '22 09:02 irfanHaslanded

What would you suggest this option does? There is a single SHM segment meaning the best I could do is filter only the data for each subscription and then write the union of them into the SHM. Meaning, for example, if there is a subscription to the whole module, this option would have no effect for any other subscriptions for the same module no matter their XPath.

Also, can you elaborate on the scalability problem? What now happens for your subscriptions is that they detect a new event, parse the diff (changed data) and filter it all out so no callback is called. Like I said, the only optimization possible is parsing less data (which is the expensive operation here) but only if specific conditions are met. This design (with a single SHM) was actually meant to solve any scalability problems because it does not matter whether you have a single subscription or 100 (for a module), the originator of the change does the exact same amount of work.

michalvasko avatar Feb 16 '22 09:02 michalvasko

Suppose there is an app that makes frequent changes in a module, in xpaths x1, x2 and x3. Let us say that there are 10 subscribers only interested in a xpath x10. With the current design, all subscribers are notified.

These 10 subscribers need not be notified at all about this event. With the new option SR_SUBSCR_PRODUCER_FILTERING, There will be no change to the single shm segment logic. Only change would be that, these 10 subscribers which only care about x10 changes will not be notified unless there is actually a change relevant to them.

irfanHaslanded avatar Feb 16 '22 14:02 irfanHaslanded

This is simply not going to work. Here is the scenario.

Every subscription has its event pipe (for notifying) but you can have 2 subscription for a single structure (sharing the event pipe). If a change is made, the event pipe is written into and there are 2 subscribers expected to process the change. Lets say subscription A used xpath X1, subscription B xpath X2, and a change of X2 was generated. Currently, on the other end the pipe would be read from, subscription A would process the event by parsing the data and after using its xpath learning that it is not interested in the change but still decreasing the subscriber_count. Then subscription B would process the event and call the callback after learning its xpath matches, leaving subscriber_count 0 and hence letting the originator know the event was handled.

It should now be obvious that if the flag would be used and worked the way you described, subscriber B would not even get to processing the event because the originator currently simply does not have a way of identifying the target subscribers. My point is that it cannot work the way you keep describing it but I am not saying that a more complex solution of the problem would not be possible. But it would need proper design.

Actually, the scenario may even be more generic, it is enough that a subscriber is handling more events because it is notified whenever an event occurs on any of its subscriptions and then all the subscriptions are checked for new events.

michalvasko avatar Feb 17 '22 09:02 michalvasko

Thanks for your answer. I have a draft PR with a unit-test with multiple subscription xpaths within a single subscription context. Please let me know if this is the situation you are describing which shouldn't be working. If there is some scenario that will still not work with these changes, I would like to correct my changes.

https://github.com/sysrepo/sysrepo/pull/2730

irfanHaslanded avatar Feb 21 '22 14:02 irfanHaslanded

I have replied in the PR but I would like to add some context. The current design is intentional (but also seemed natural at the time of implementation) because it keeps the work of the producer constant no matter how many subscribers there are. Also, every subscriber will filter the data only once so the work is evenly distributed among the producer and consumers.

While this sounds reasonable in theory, as probably observed in your use-cases, in practice it does not work. Mainly because only a single XPath evaluation for each subscriber with XPath is saved for the producer while redundant data parsing (and XPath filtering) is required for every subscriber not interested in the change. This simply does not compare because parsing data is many times slower than XPath filtering.

However, with the current architecture of the communication between producer and subscribers it is not trivial to implement this optimization even though you seem to think so. It needs to be properly designed not to break any functionality or features.

michalvasko avatar Feb 22 '22 13:02 michalvasko