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

Increase performance of comments tab #4473

Merged
merged 21 commits into from
Mar 30, 2020
Merged

Increase performance of comments tab #4473

merged 21 commits into from
Mar 30, 2020

Conversation

philippotto
Copy link
Member

@philippotto philippotto commented Mar 11, 2020

Increases the performance of the comment tab by:

  • not rendering the comment tab if it's not visible due to the golden layout
  • skipping recalculation of the meta data when the diff of the previous to the current skeleton tracing only contains irrelevant actions
  • speeding up the calculation of the meta data by ditching reduce and concat in favor of a simple for loop with push

Additionally, I added some other minor improvements concerning the import of NMLs (faster node lookup and skipping id reassignments when the tracing is empty).

URL of deployed dev instance (used for testing):

  • https://___.webknossos.xyz

Steps to test:

  • Test the usual comment handling (adding, editing, deleting comments) and skeleton interaction (switching active nodes, deleting adding nodes etc.)
  • I tested the branch with a tracing which has lots of comments and the time to generate the comment tab "metadata" decreased from ~500ms to ~25ms. Also, I verified that the calculation of the meta data is skipped, when the comments tab is inactive.

Issues:


@philippotto philippotto self-assigned this Mar 11, 2020
@philippotto
Copy link
Member Author

@MichaelBuessemeyer ping :)

@MichaelBuessemeyer
Copy link
Contributor

MichaelBuessemeyer commented Mar 24, 2020

pong :)

sorry, looks like I missed an email 🤔. I'll do it soon 👍

Copy link
Contributor

@MichaelBuessemeyer MichaelBuessemeyer left a comment

Choose a reason for hiding this comment

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

While testing I encountered some weird error causing the whole page to crash.
To see the error please open this link and select the comments tab.

What I did to create the error:

  • I created some trees with comments.
  • When I tried to edit a comment using the modal the whole page crashed when I entered a newline in the modal.
  • Just entering letters and spaces does not create this error

I just tested to reproduce the error again and it is very easy when following the two steps above.
Could you please investigate this error? I tried finding the error by just looking at the code changes but could not identify something relevant 😕.

The master seems to work fine and does not have this error.

The rest looks fine to me 😄
Awesome work and speed up 🚀

@philippotto
Copy link
Member Author

While testing I encountered some weird error causing the whole page to crash.
To see the error please open this link and select the comments tab.

What I did to create the error:

  • I created some trees with comments.
  • When I tried to edit a comment using the modal the whole page crashed when I entered a newline in the modal.
  • Just entering letters and spaces does not create this error

I just tested to reproduce the error again and it is very easy when following the two steps above.
Could you please investigate this error? I tried finding the error by just looking at the code changes but could not identify something relevant .

Thank you for testing this so thoroughly and also for the detailed steps to reproduce 👍 Very helpful! And I'm glad that this didn't go to production with this bug.

I found that the renaming of the CSS class broke things, because the string was not DRY in the code. So, I unified this and the bug should be fixed now.

I also added some minor performance improvements to this PR (see f7f71eb). Would be cool if you could have another glance at that :)

@philippotto
Copy link
Member Author

I added another improvement for the case where the current tracing is empty. In that case, id reassignment is not necessary which makes the NML import twice as fast.

Copy link
Contributor

@MichaelBuessemeyer MichaelBuessemeyer left a comment

Choose a reason for hiding this comment

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

The whole PR looks awesome. The same goes for f7f71eb and 72c5334. 🚀

I tested the NML import and it still works as expected. 👍 But I did not do a benchmark for this.

However, while testing the importing NML's I noticed that if the NML contains comments, the comment tab is not updated and does not show the imported node. Only after performing on relevant action on the tracing that triggers the rerendering of the comment tab, the new trees with comments are displayed.

I had a quick look at this and found the reason: The action "createTree" is being ignored although it has the comments included:


updateActions:​
[{name: "createTree",
 value: {
     branchPoints: Array [],​​​
     color: Array(3) [ 1, 0, 0 ],​​​
 -->    comments: Array(5) [ {…}, {…}, {…}, … ],​​​
     groupId: null,​​​
     id: 1,​​​
     isVisible: true,​​​
     name: "Tree001",​​​
     timestamp: 1585485798518,​​​
     updatedId: undefined}}, ...]

I added a quick and but not so nice solution for this.

@MichaelBuessemeyer MichaelBuessemeyer self-requested a review March 30, 2020 12:25
Copy link
Contributor

@MichaelBuessemeyer MichaelBuessemeyer left a comment

Choose a reason for hiding this comment

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

Awesome 🕺 ✔️ Let's merge these performance improvements

@bulldozer-boy bulldozer-boy bot merged commit fc378a3 into master Mar 30, 2020
@bulldozer-boy bulldozer-boy bot deleted the comments-perf branch March 30, 2020 13:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The comment tab can be quite slow when there are lots of comments
2 participants