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

Desktop: UI response gets worse for each additional plugin of some type #6394

Closed
ken1kob opened this issue Apr 11, 2022 · 17 comments
Closed
Labels
bug It's a bug desktop All desktop platforms high High priority issues plugins Anything related to Joplin's plugin system

Comments

@ken1kob
Copy link
Contributor

ken1kob commented Apr 11, 2022

As reported in the topic of discourse, for each plugin of some type is added, Joplin app's UI response gets worse to a negligible degree. This problem discourages people from using some beneficial plugins.

For example, the below is performance measurement results of note switching in my PC (its detail is explained later).

Plugins Switching time (msec) within a notebook Switching time (msec) between notebooks
no 202 1150
Note Tabs 478 1587
Note Tabs + Outline 706 1815
Note Tabs + Outline + 8 plugins 807 1876

It shows:

  • Some plugins have heavy overheads (200-400 msec).
  • Overheads of other plugins are enough small.

This performance degradation is not only for the above two plugins. And, this performance degradation affects on not only note switching time but almost UI activities such as editing.

What is expected

When a plugin of this symptom-causing type is added, the UI response degradation should be comparable to other plugins.

Environment

  • Joplin version: 2.7.15
  • Platform: any desktop

How to reproduce

Just add Outline or Note Tabs plugin.

@ken1kob ken1kob added the bug It's a bug label Apr 11, 2022
@ken1kob
Copy link
Contributor Author

ken1kob commented Apr 11, 2022

The detail of the above example is here.

  • How to measure
  • The measured note is "1. Welcome to Joplin!".
  • Spellchecking is not used.
  • How to switch between notebooks: Sidebar
  • PC spec: Core i5-4670 (CPU Mark=5403), 32GB memory, 4TB SSD
  • OS: Win10
  • Of course, all measurements are subject to error.

Full results:

Plugins #plugins Time (msec) within a notebook Time (msec) between notebooks Large overhead
No plugins 0 202 1150
+Note Tabs 1 478 1587
+Outline 2 706 1815
+Favorites 3 775 1768
+Math Mode 4 760 1807
+Markdown Table: Colorize 5 750 1959
+Markdown Table: Sortable 6 764 1900
+turnToCharts 7 863 2035
+Templates 8 807 1876
+Table Formatter Plugin 9 838 2036
+Resource Search Plugin 10 859 1915
+Quick Links 11 827 2061
+Persistent Editor Layout 12 802 1960
+Note overview 13 863 2025
+Kanban 14 854 2032
+Insert Date 15 837 2059
+Conflict Resolution 16 889 2182

Edit (2022-04-13): The measured values of Conflict Resolution were incorrect and have been corrected.

@laurent22
Copy link
Owner

laurent22 commented Apr 11, 2022

Strange that Conflict Resolution would have a high overhead since it doesn't have a webview.

In general I wonder if the overhead is due to webviews being created, then destroyed, then created again. And in that case perhaps a solution would be to keep a pool of webviews that we show/hide (without destroying them) depending on the plugins.

@ken1kob
Copy link
Contributor Author

ken1kob commented Apr 12, 2022

Strange that Conflict Resolution would have a high overhead since it doesn't have a webview.

I agree. I measured it several times again and watched it carefully in Performance Analyazer. As a result, Conflict Resolution does not have a significant overhead. I've corrected the table.

@ken1kob
Copy link
Contributor Author

ken1kob commented Apr 12, 2022

Analysis

To analyze the cause, I inspected the difference between no-plugins and one-plugin (Note Tabs) using Performance Analyzer.

The below is the comparison of two Performance Analyzer views, note switching (A) with no plugins and (B) with Note Tabs plugin. The measured note is "1. Welcome to Joplin!", and each switching was performed within a notebook.

performance-analyzer

(A) takes 254 ms, and (B) takes 554 ms. The difference is 300 ms. From the views, it is found that parts of (A) are also parts of (B). But, some parts of (B) are not contained in (A). The additions of (B) are two blue regions (plugin parts) and two red regions (re-rendering parts). It is natural that plugin parts exist, but it is strange that re-rendering parts are so large. By assessing the re-rendering parts, it is found they contains re-rendering of most UI components. It is excessive. Even if Note Tabs plugin exists, note switching within a notebooks needs much less components' re-rendering. Additionally, half time of re-rendering parts are occupied by MenuBar's update (updateMenu() and scheduleUpdate()). It is also strange that they are not needed to be re-rendered in this case.

These excessive re-renderings occupy almost all of Note Tab plugin's overhead.

@ken1kob
Copy link
Contributor Author

ken1kob commented Apr 13, 2022

Cause

The cause of the above excessive (= redundant) re-rendering is false positive dependencies. Most of React component in Joplin refer a plugin object prop. So, if a part of the plugin prop changes, most components are re-rendered. Although most parts of plugin prop are almost stable and rarely changed, html attribute, an html content of plugin's panel component, could be changed more frequently. If html attribute is changed in response to frequent events such as switching notes and a change of a note's content, most components result in being re-rendered.

The following is an illustration of a typical case of this issue. Its scenario is:

  • Step (1): a note in NoteEditor is switched, and then Plugin1 and Plugin3 react the change.
  • Step (2): Plugin1 changes its own html content. Since most UI components depend on it, they are re-rendered.
  • Step (3): Plugin3 changes its own html content. Since most UI components depend on it, they are re-rendered again.

avalanche-of-re-rendering

In this way, an avalanche effect of re-rendering occurs and results in a bad UI response.

Plugins affected by this issue

The bug is in the app itself, not in any plugins. And not all plugins are affected by the bug.

An affected plugin is a plugin using joplin.views.panels which changes the content of its panel in response to frequent events such as changing a note's content and switching notes.

@laurent22
Copy link
Owner

Thanks for the detailed analysis, as always it's very useful to see what's happening. It annoys me that changing the view HTML would update these components because it means they are not well written - they should just get from the state the properties they need and nothing more. While here they probably take the whole plugin states and that causes a problem.

I guess you at least partially solved this inn #5770? The issue with this pull request is that it includes multiple optimisations, including some unrelated to that plugin view HTML issue. So if that PR helps fix the issue, any chance you could refocus it purely to this view HTML issue?

And of course we can look at the other optimisations in separate pull requests.

@laurent22 laurent22 added desktop All desktop platforms high High priority issues plugins Anything related to Joplin's plugin system labels Apr 14, 2022
@laurent22
Copy link
Owner

I've tried to fix the menu bar issue by splitting the plugin states into state and views as we discussed before, but that required changing many files in many places.

In the end I went for using a ref, just three lines of changes, and in that particular case this is fine (although not ideal). As far as I can see the menu bar now no longer re-render when switching notes.

@laurent22 laurent22 reopened this Apr 14, 2022
@laurent22
Copy link
Owner

Do you think other parts of the UI are affected by needless re-rendering?

I've checked the sidebar, and it's already using a plugin ref so it should be fine. The note list needs to re-render anyway because the selection changes when changing notes, and same for the editor.

@ken1kob
Copy link
Contributor Author

ken1kob commented Apr 14, 2022

Treatment

I guess you at least partially solved this inn #5770?

The item 1 of PR #5770 will fix this issue. The detail is described there.

So if that PR helps fix the issue, any chance you could refocus it purely to this view HTML issue?

I see. I'll separate it from PR #5770 to PR #6409.

The remaining item 2-4 of PR#5770 will be covered by issue #6386. Since PR #5770 completely includes PR #6409, it can be safely applied later.

@ken1kob
Copy link
Contributor Author

ken1kob commented Apr 14, 2022

In the end I went for using a ref, just three lines of changes, and in that particular case this is fine (although not ideal). As far as I can see the menu bar now no longer re-render when switching notes.

Oh...

Do you think other parts of the UI are affected by needless re-rendering?

I've checked the sidebar, and it's already using a plugin ref so it should be fine. The note list needs to re-render anyway because the selection changes when changing notes, and same for the editor.

For #5770 (and #6409), I had checked codes in the list here, and I know #5770 works fine for them.
But I cannot immediately tell an approach like 343b81a works fine or not for many components.

@ken1kob
Copy link
Contributor Author

ken1kob commented Apr 14, 2022

In the end I went for using a ref, just three lines of changes, and in that particular case this is fine (although not ideal). As far as I can see the menu bar now no longer re-render when switching notes.

For this issue, that is, for the matter of plugins, 343b81a looks good.

For the matter of switching notes, one more concern will be needed. Because re-rendering of MenuBar also occurs when switching notebooks.

I have a question. Did you switch notes within a notebook or between notebooks? If you didn't try between notebooks, could you try it? Re-rendering of MenuBar might happen.

@laurent22
Copy link
Owner

Good point, I've only switched between notes. I'll check switching between notebooks too.

@laurent22
Copy link
Owner

No re-rendering when switching notebooks while Note Tabs is running, so it seems to be fine. Or did you notice this happening with a particular plugin?

@ken1kob
Copy link
Contributor Author

ken1kob commented Apr 14, 2022

Do you think other parts of the UI are affected by needless re-rendering?

I've checked the sidebar, and it's already using a plugin ref so it should be fine. The note list needs to re-render anyway because the selection changes when changing notes, and same for the editor.

As I wrote in #5770, the layout of MainScreen would also be affected. If it happens, all components under MainScreen will be re-rendered.

Another concern is when typing. For example, if you edit a new header line, Outline plugin will raise an re-rendering avalanche. And, if you edit - [x], Note Tabs plugin will raise it, if "Show checklist completion status" option is enabled. In such cases, NoteList shouldn't be re-rendered.

Moreover, if NoteEditor is re-rendered, its children such as NoteTitleBar, Toolbar, and NoteSearchBar are also re-rendered.

@laurent22
Copy link
Owner

The note list appears to be re-rendered because props.size and props.notes are changed. props.size is probably just a mistake that could be fixed easily. For props.notes it's harder because the currently edited note changes, which in turns changes the notes array too. It's another case of a component requiring more state than it actually needs.

@ken1kob
Copy link
Contributor Author

ken1kob commented Apr 14, 2022

No re-rendering when switching notebooks while Note Tabs is running, so it seems to be fine. Or did you notice this happening with a particular plugin?

Re-rendering MenuBar when notebooks are switched is not relevant to plugins.

I tried the latest dev with 343b81a. When notebooks are switched, MenuBar's updateMenuBar() becomes not called because of 343b81a. But MenuBar's scheduleUpdate() is still called.

BTW, exactly speaking, MenuBar is re-rendered even if 343b81a is applied. Re-rendering is performed, but heavy updateMenuBar() is not called. On the other hand, PR #6409 prevents re-rendering.

@ken1kob
Copy link
Contributor Author

ken1kob commented Nov 15, 2022

Since #6409 was merged, this issue can be closed.

@ken1kob ken1kob closed this as completed Nov 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug It's a bug desktop All desktop platforms high High priority issues plugins Anything related to Joplin's plugin system
Projects
None yet
Development

No branches or pull requests

2 participants