corenova / yang-js

YANG parser and composer
Apache License 2.0
56 stars 18 forks source link

performence issue with big amount of list data #73

Closed quantang closed 5 years ago

quantang commented 5 years ago

We are using yang-js to deal with network topology data and getting some performance issues when the amount of data reaches a certain level, such as we have a network entity which has 800 nodes and 2000 links. We did some perf profiling and identified the following potential improvements we may have:

  1. Do not valid a "leafref" element if the "require-instance" is false. We have some "leafref" elements with "require-instance" false, however, the yang-js still takes a lot of time trying to find the matching entity, which could be skipped.

  2. XPath.process function. There is a reduce function against data inside, which calls Array.concat function in the reducer function. It will take lots of time if the data is a big array, as the Array.concat will create a new array every time. I tried to refine it and push the new elements into an existing array, which makes it much better.

  3. There is no key index for list element. The perf profiling shows some performance issue on Property.in and Property.merge, which should be related to that the yang-js does not have the internal key index for the list element. As it needs to iterate all the elements to make sure there is no duplicate key for the "merge" operation or find the right list item based on a given key for "in" operation.

Cheers.

sekur commented 5 years ago

Hi @quantang, thanks for this. Its perfect timing since this was exactly the area I’m currently working on optimizing. For one, I’ve recently shifted the internal List implementation to use Map instead of pure Array. This should help with the merge operation.

Another optimization is in how we handle “config false” data. In this case, instead of attaching Propertly instance to every node in the hierarchy, I just do a new .validate call which only performs schema validity test.

I also have added test cases to measure list operation timing data so I can track improvements along the way.

As it currently stands, the performance for larger set of list entries is subpar and I’m focused on optimizing it.

sekur commented 5 years ago

BTW - try out the latest published version. It’s still not where I want it to be, but should be a bit snappier.

quantang commented 5 years ago

Hi @saintkepha, I am sorry that I didn't try the latest version before raising the issue. I will have a try soon and get back to you if I have any finding there. Cheers.

sekur commented 5 years ago

Ok, just published 0.17.24. This version should have around 50% performance improvement dealing with list. No worries on the latest version thing, I've been pushing list related improvements over the past couple of days only.

sekur commented 5 years ago

Also, I haven't leveraged the new list Map capabilities for the in and find operations yet. Once that's in place, direct key based lookups should be much much faster. Another todo is related to #68 where leafref requires an extra '..'

sekur commented 5 years ago

Actually, the extra '..' leafref inside list might already be fixed... I'll add a testcase for it but just wanted you to be aware of it since I recall you had to do some workaround on the ietf-network-topology schema before.

quantang commented 5 years ago

Hi @saintkepha , I saw your change introduced some new types, List and ListItem, which extends Property. However, ListItem returns nothing on the toJSON(), is it expected?

Cheers.

sekur commented 5 years ago

Ah, no that must be a bug :-) On Wed, Oct 10, 2018 at 8:26 PM Quan Tang notifications@github.com wrote:

Hi @saintkepha https://github.com/saintkepha , I saw your change introduced some new types, List and ListItem, which extends Property. However, ListItem returns nothing on the toJSON(), is it expected?

Cheers.

— You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub https://github.com/corenova/yang-js/issues/73#issuecomment-428807675, or mute the thread https://github.com/notifications/unsubscribe-auth/AA6KbC8v87ZkipEkHmIqkc-N6Isuw-aqks5ujrp-gaJpZM4XWmjS .

-- Peter Lee Chief Executive Officer Corenova Technologies, Inc. +1-310-400-6450 linkedin.com/in/peter-k-lee

quantang commented 5 years ago

Never mind, I just opened #74 for tracking the toJSON() issue on ListItem. Cheers.

quantang commented 5 years ago

Hi @saintkepha , Great job! The latest version looks awesome.

Based on my perf profiling, the only rest issue, as you already mentioned above, should be the Property.in, which will further get into XPath.process and Filter.transform. It looks like some element iteration and key comparison happens there.

I hope it can help you make it better. :P Cheers.

sekur commented 5 years ago

Closing since the last performance improvement item captured as a separate issue.