MAYA-110878 - handle multiple deferred notifs for single path #1519
Conversation
…for attributes which don't exist, and drops some notifications * Handle multiple deferred notifications for a single path (keeping the original notif order).
| // 1) Order of notifs must be maintained. | ||
| // 2) Allow notif with same path, but different token. At worst the check is linear | ||
| // in the size of the vector (which is the same as an unordered_multimap). | ||
| std::vector<std::pair<Ufe::Path, TfToken>> pendingAttributeChangedNotifications; |
There was a problem hiding this comment.
Change is pretty simple. I just changed the container to vector (for reasons give here). I ran all the UFE tests and they all passed with no test changes required. I believe this is due to the previous changes around line 420 which ignore inert prim changes.
There was a problem hiding this comment.
I think that vector will not be fast enough for the duplicate notification elimination at line 95. While the worst case is the same, the average case or unordered_map is constant while the average case the vector is linear in the size of the vector.
I think to do better you can have a second container which is a std::unordered_set< std::pair<Ufe::Path, TfToken> >. The idea is you use the set to test if the std::pair<Ufe::Path, TfToken> has already been saved in the vector in constant time, and you use the vector to maintain ordering. We trade off a little constant time & some space, but we avoid the linear search.
There was a problem hiding this comment.
I do see your point, but I wonder if its really needed. I added some tf_debug statements to print out when a notif gets delayed. I ran all the "UFE" tests and the largest size the pendingAttributeChangedNotifications vector got was only 2. Granted the tests aren't real-world data, but I think that when normally running the notif guard will expire and send the pending notifs long before they ever have a chance to build up. @ppt-adsk What do you think?
There was a problem hiding this comment.
Hard to say, I tend to think @seando-adsk you're right, but it's a code complexity vs. performance guarantee tradeoff. Can we perhaps put up a message if the vector size grows above some threshold?
There was a problem hiding this comment.
The case I'm worried about is someone writing a script that iterates over everything in the scene and changes each one a little bit, say changing the purpose of each rprim. That is going to create N messages and have O(N^2) complexity because it is doing a linear search.
If you really don't like the unordered_set we can leave it as-is and ask QA to play around with some heavy scenes and see if they hit any performance issues.
There was a problem hiding this comment.
If you look here you can see where we use the notif guard - basically when setting a single usd attribute. In your scenario on each prim we change a little bit we will enter/leave a notif guard. So I don't ever see this notif guard growing beyond just a few notifs.
There was a problem hiding this comment.
Okay sounds good to me!
| sendValueChanged(ufePath, changedPath.GetNameToken()); | ||
| } | ||
| #endif | ||
| UFE_V2(valueChanged(ufePath, changedPath.GetNameToken());) |
There was a problem hiding this comment.
Use the helper func which checks the notif guard. The previous code would also not add this value changed to the deferred notifs if we were in a guard.
| // 1) Order of notifs must be maintained. | ||
| // 2) Allow notif with same path, but different token. At worst the check is linear | ||
| // in the size of the vector (which is the same as an unordered_multimap). | ||
| std::vector<std::pair<Ufe::Path, TfToken>> pendingAttributeChangedNotifications; |
There was a problem hiding this comment.
I think that vector will not be fast enough for the duplicate notification elimination at line 95. While the worst case is the same, the average case or unordered_map is constant while the average case the vector is linear in the size of the vector.
I think to do better you can have a second container which is a std::unordered_set< std::pair<Ufe::Path, TfToken> >. The idea is you use the set to test if the std::pair<Ufe::Path, TfToken> has already been saved in the vector in constant time, and you use the vector to maintain ordering. We trade off a little constant time & some space, but we avoid the linear search.
MAYA-110878 - MayaUSD sends some UFE attribute changed notifications for attributes which don't exist, and drops some notifications