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

enchance/solve bad performance when clicking metric button #925

Merged
merged 6 commits into from
Feb 15, 2023

Conversation

cyfml
Copy link
Contributor

@cyfml cyfml commented Feb 12, 2023

This PR fixed bad performance when clicking metric button. When there are more than thousand comparisons, it becomes very slow. The reason is that, web page needs to re-render thousand of DOM nodes (<tr>). Because all comparisons shows in the ComparisonsTable . It wastes too much time and memory. I found it takes 1 second or even several seconds to load page in Vue dev tool, when there are over 1K comparisons. This problem should be considered at designing, but not too late.

Actually, there are two solutions;

  1. paging:

show some parts of comparisons in one page. This solution dosen't fit our design. Because scrolling will be more suitable to see the coherence of similarity. From high to low.

  1. virtual scroll:

Virtual scrolling enables a performant way to simulate all items being rendered by making the height of the container element the same as the height of a total number of elements to be rendered, and then only rendering the items in view. So performance is really good, even there are over 10K records. So I choose this way to improve the performance. But we need install this plugin (vue-virtual-scroller@next).

I keep the layout of this ComparisonsTable as before as possible. Because original table layout must be replaced when using this plugin.

Additionally, I also made the route lazy loaded.

@sebinside sebinside self-requested a review February 13, 2023 13:22
Copy link
Member

@sebinside sebinside left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First of all, I like your solution and it greatly improves the performance, thank you! However, I have some comments regarding the code quality. Especially the view breaks if the content of the table cells get to long, which is unfortunately a realistic use case. Nevertheless, looking forward to include this PR!

@sebinside
Copy link
Member

@cyfml: Here is an example of a result zip file that breaks the new view. It's just the PartialPlagiarism test case with longer (realistic) submission names.
result.zip

@sebinside
Copy link
Member

Ah, and of course please also update to the current state of the develop branch.

@cyfml
Copy link
Contributor Author

cyfml commented Feb 13, 2023

First of all, I like your solution and it greatly improves the performance, thank you! However, I have some comments regarding the code quality. Especially the view breaks if the content of the table cells get to long, which is unfortunately a realistic use case. Nevertheless, looking forward to include this PR!

@sebinside Thanks, the difficult of this is it doesn't fit for table, it's more suitable for list. Because in table structure, i add the new component RecycleScroller, it breaks the structure of table. So the content is not aligned. And this method must use fixed size of table container.

@sebinside
Copy link
Member

@cyfml What about DynamicScroller then? The benefit is described as The main use case for this is not knowing the height of the items in advance

@cyfml
Copy link
Contributor Author

cyfml commented Feb 13, 2023

@sebinside I am trying.

@cyfml
Copy link
Contributor Author

cyfml commented Feb 13, 2023

@sebinside All things are done. Please review it and test them.

@cyfml cyfml requested a review from sebinside February 13, 2023 17:48
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

81.8% 81.8% Coverage
0.0% 0.0% Duplication

@sebinside
Copy link
Member

Good work, I tested it with 3000 comparisons and did not encounter any problems. Thank you!

@sebinside sebinside merged commit 02573ce into jplag:develop Feb 15, 2023
@cyfml
Copy link
Contributor Author

cyfml commented Feb 15, 2023

@sebinside Thanks. Could you add an item about this problem in issues list? When i write documentation, i have something to look.

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

Successfully merging this pull request may close these issues.

2 participants