TypeFox / yang-lsp

A Language Server for YANG
http://www.yang-central.org
Apache License 2.0
51 stars 13 forks source link

XpathResolver.findNodes() could cause extreme bad performance when resolve complex long xpath. #154

Closed huyuwen closed 5 years ago

huyuwen commented 5 years ago

Recently, we got a bunch of yang modules which contains quite complex xpath. All these xpath are distributed in 6 - 8 modules as part of Augment, Deviation, Must or When. They have cross reference to each other as well. When resolving one of these modules, it takes 200+ SEC to resolving only one xpath that has 6 fragments. And that module has 50 xpath which also have 6 fragments. It becomes a disaster to validate the module because there has no chance to cancel a IScopeContext.resolveAll() method. See https://github.com/theia-ide/yang-lsp/blob/2aca17f7d3787bd70d3def90b4b104c5c3699fbc/yang-lsp/io.typefox.yang/src/main/java/io/typefox/yang/validation/ResourceValidator.xtend#L31

So I prepared a test case to reproduce the performance issue. I found when resolving the Xpath that I mentioned above, the return value (elements) of XpathResolver.findNodes() actually has more than one item, but they all point to a same EObjectDescription object. https://github.com/theia-ide/yang-lsp/blob/2aca17f7d3787bd70d3def90b4b104c5c3699fbc/yang-lsp/io.typefox.yang/src/main/java/io/typefox/yang/scoping/xpath/XpathResolver.xtend#L454 If we take that xpath (6 fragment) as example, 1st fragment will return 8 duplicate items, 2nd fragment will return 64 duplicate items, the 6th fragment will return 32768 duplicate items. The number of duplicate items is decide by the reference count of that fragment, I guess.

I didn't figure out why there will have so many duplicate items, maybe due to overlap NodeScope and complex cross reference relation between the 6 - 8 modules. Therefore, the simplest way to avoid all those duplicate EObjectDescription to be loaded time by time. It is better to convert the original elements List to Set first, and then to List again.

         return elements.toSet.toList

I tried locally with this solution. When resolving the same xpath that has 6 fragments, it only takes about 400 ms. Then I re-run all other tests, there has no failure test cases. So I think toSet() could be an alternative to fix this fatal performance issue. Meanwhile, I think the following result may also need to apply with the same steps. https://github.com/theia-ide/yang-lsp/blob/2aca17f7d3787bd70d3def90b4b104c5c3699fbc/yang-lsp/io.typefox.yang/src/main/java/io/typefox/yang/scoping/xpath/XpathResolver.xtend#L417

kittaakos commented 5 years ago

@huyuwen, I have created a PR with your proposed changes.