Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RVK instance is slow #513

Closed
nichtich opened this issue Nov 6, 2019 · 5 comments
Closed

RVK instance is slow #513

nichtich opened this issue Nov 6, 2019 · 5 comments
Labels
bug Something is broken
Milestone

Comments

@nichtich
Copy link
Member

nichtich commented Nov 6, 2019

The instance at https://coli-conc.gbv.de/cocoda/rvk/ is slower than dev, this may be caused by concept lists. We could limit maximum number of concept list entries if this helps.

@nichtich nichtich added this to the 1.2.0 milestone Nov 6, 2019
@nichtich nichtich changed the title RVK instance is slow and RVK instance is slow Nov 6, 2019
@nichtich nichtich added the bug Something is broken label Nov 6, 2019
@stefandesu
Copy link
Member

stefandesu commented Nov 7, 2019

I can imagine that this has to do with the concept lists, especially because the one list we have in there has 250 entries, which means 250 additional instances ConceptListItem. We could limit the number of entries, but that would impede the concept list feature. Maybe it would be better to add #4 after all...

Alternatively, I could try to figure out how to make concept lists more efficient. Maybe by creating the whole list in-line instead of using components. If you think about it, we have 250 instances of ConceptListItem which in this case add 500 instances of ItemName (250 for the concepts and 250 for the scheme in front of the concepts which seems very unnecessary), as well as 250 instances of LoadingIndicator which are shown when narrower concepts are loaded (also seems very unnecessary considering that there are no narrower concepts in a flat list). Each ItemName in turn has a router-link as well as notation-text (which is defined in ItemName). If you add this up, we have a total of a whopping 2000 Vue components if I'm not mistaken, that is simply way way too much for a single list...

@nichtich
Copy link
Member Author

nichtich commented Nov 7, 2019

In lists we don't need full functionality of ConceptListItem. At least the Concept Scheme notation could be fixed. It would also be ok to not update labels when the language is changed. What's needed is dynamic indicator which concepts have been mapped. With #508 we could get dynamic lists via API that are likely smaller, e.g. 40 recently mapped concepts etc. To manually update a list's view we could add a reload button at the bottom.

stefandesu added a commit that referenced this issue Nov 8, 2019
Functionality is moved to $util.notation.
stefandesu added a commit that referenced this issue Nov 8, 2019
Instead of using ItemName, the notation and prefLabel are included directly. This should help reduce overhead by having too many Vue components loaded.
@stefandesu
Copy link
Member

In lists we don't need full functionality of ConceptListItem. At least the Concept Scheme notation could be fixed. It would also be ok to not update labels when the language is changed.

What I now did was to replace ItemName with getting the notation and prefLabel directly. Also, LoadingIndicator is now added with a v-if so it's only there when it's actually needed. I didn't think it was reasonable to have two different versions of ConceptListItem depending on whether it's a list or a tree view. Also, not updating labels when the language is changed is way more complicated than updating them because I don't have to do anything to have them updated. Not having them updated would mean reimplementing a whole bunch of things. I also don't think it would help much with performance.

We could take this one step further and include everything that's in ConceptListItem into ConceptList because that's the only place where the component is used.

What's needed is dynamic indicator which concepts have been mapped.

This is fortunately outside of ItemName and was therefore not affected by my changes.

To manually update a list's view we could add a reload button at the bottom.

I'll add this for lists then are loaded from a URL.

@stefandesu
Copy link
Member

Oh btw, I can't really say if all this actually helped with performance because I personally don't see any performance problems that seem related to long lists.

@stefandesu
Copy link
Member

Closing in favor of #208.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something is broken
Projects
None yet
Development

No branches or pull requests

2 participants