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: Note switching time is too slow #6386

Open
ken1kob opened this issue Apr 10, 2022 · 21 comments
Open

Desktop: Note switching time is too slow #6386

ken1kob opened this issue Apr 10, 2022 · 21 comments
Labels
desktop All desktop platforms enhancement Feature requests and code enhancements

Comments

@ken1kob
Copy link
Contributor

ken1kob commented Apr 10, 2022

As reported in the topic of discourse, some people are suffering from Joplin's UI performance. Since the specs of PCs are in a very wide range (5x), not all people feel UX response is comfortable. In actually, some people forgo using plugins to mitigate UI performance problems.

This issue focuses on one of the key UI performance aspects, note switching time. The following chart is the summary of the collection of many people's reports in the topic (thanks!). It shows that note switching time is over 1 sec in not a little range of PCs. Especially, it is found that note switching between different notebooks takes over 1.5 sec even for fast PCs, if some plugins are used.

note-switching-time

What is expected

For many users' PCs, note switching time becomes less than 1 sec. Because it is often said that "1.0 second is about the limit for the user's flow of thought to stay uninterrupted".

Note:

  • Since PC performances are in a very wide range, "1 sec" is not for all PCs.
  • "1 sec" is not a rigid goal. If it costs too much to achieve, it should be relaxed to a realistic goal.

Environment

  • Joplin version: any (2.7.x)
  • Platform: any desktop
@ken1kob ken1kob added the bug It's a bug label Apr 10, 2022
@ken1kob
Copy link
Contributor Author

ken1kob commented Apr 10, 2022

Performance bottleneck analysis

I analyzed performance bottlenecks of Joplin UI. Current findings from the discourse topic and my analysis are as follows.

These details will be described in later posts, specific issues, or upcoming PRs.

Performance measurement

To measure note switching time, Performance Analyzer of Electron is used.
The measurement is performed when Markdown Editor and Viewer (Split Layout) is used. And, there are some parameters.

  • Target note name (typically, "1. Welcome to Joplin!" is used.)
  • Whether to switch notes within a notebook or between notebooks
  • What plugins used
    • Notable plugins are Note Tabs plugin and Outline plugin.
  • If spellchecking used
  • How to switch notes: Sidebar, NoteList Panel, Note Tabs, and so on.
  • Environment: CPU spec, OS

The measurement procedure is described in this discourse post.

@ken1kob
Copy link
Contributor Author

ken1kob commented Apr 18, 2022

The two performance bugs (#6394 and #6416) are close to being solved. The next is to solve performance bottlenecks in this issue.

@laurent22, shall I make a large PR packaging several optimizations or several PRs per a single optimization? Which way would you prefer?

@laurent22
Copy link
Owner

Yes, a single optimisation per pull request please, it makes it much easier to review.

@ken1kob
Copy link
Contributor Author

ken1kob commented Apr 20, 2022

About inefficient props update

In React, props and states are used to decide which components are needed to be updated. So, setting props should be sufficiently fast. WhenClause and WhenClauseContext are frequently used for setting props. However, their operations are heavy. Especially, WhenClause needs parsing to be constructed.

PR #6437 resolves these bottlenecks. Its detail is there.

@laurent22
Copy link
Owner

But WhenClause expression parsing is already cached I believe? The rest is constructing a key/value dictionary which I assume is not that slow.

@ken1kob
Copy link
Contributor Author

ken1kob commented Apr 21, 2022

But WhenClause expression parsing is already cached I believe? The rest is constructing a key/value dictionary which I assume is not that slow.

Partially yes, but no in totality.
WhenClause has two layers: The outer is AdvancedExpression, and the inner is VSCode's ContextExprKey. ContextExprKey instances are cached. But AdvancedExpression instances are not cached and always re-parsed when used. They are slow and take not a small time in Performance Analyzer.

@laurent22
Copy link
Owner

If parsing advanced expressions is what takes time then that's what we should be cached. I probably won't merge that WhenClause pull request for now, and if there are optimisations to make I might make them.

@ken1kob
Copy link
Contributor Author

ken1kob commented Apr 21, 2022

If parsing advanced expressions is what takes time then that's what we should be cached.

Only caching AdvancedExpression instances is not enough. Caching WhenClause instances is needed. The reason is written below.

I probably won't merge that WhenClause pull request for now, and if there are optimisations to make I might make them.

Honestly speaking, the current WhenClause has a performance BUG.
Its caching mechanism for ContextKeyExpr doesn't almost work. Because its cache (ruleCache_) is an instance member of WhenClause and is deleted immediately after used. By using Performance Analyzer, you can see parsing ContextKeyExpr is not almost skipped.
To fix this bug, caching WhenClause instances is needed as PR #6437 does. So, I think now is the best time to fix it.

BTW, creating and copying WhenCaluseContext instances are also heavy. So, they also worth being adopted.

@ken1kob
Copy link
Contributor Author

ken1kob commented Apr 23, 2022

About redundant re-rendering (false positive updates#1)

In React, the changes of useState variables always involve re-rendering of components. If unnecessary useState variables are used, they may cause redundant re-rendering.

PR #6443 suppresses such redundant re-rendering in Markdown Editor/Viewer.

@ken1kob
Copy link
Contributor Author

ken1kob commented Apr 23, 2022

About redundant rerendering (false positive dependencies#1)

In React, if unnecessary props or states are described, redundant rerendering may occur. The following PRs fix such unnecessary dependencies in MainScreen component to reduce rerendering.

@ken1kob
Copy link
Contributor Author

ken1kob commented Apr 23, 2022

Performance comparison#1

The below shows comparisons among a version before-improved, a version with two performance bug fix PRs (#6394,#6416) and a version with the two fix PRs and four improvement PRs (#6437,#6443,#6444,#6446) up to this point.

Condition Plugins Before
Time (msec)
2 PRs
Time (msec)
2+4 PRs
Time (msec)
Within a notebook no 206 162 136
Within a notebook NoteTabs + Outline 706 219 162
Between notebooks no 1150 688 587
Between notebooks NoteTabs + Outline 1815 821 670

@ken1kob
Copy link
Contributor Author

ken1kob commented Apr 24, 2022

About redundant re-rendering (false positive updates#2)

When a notebook is switched using Sidebar, for example, in the Performance Analyzer image (A) in this PR #6451, it is strange that Sidebar is re-rendered three times. In this case, data used for Sidebar don't change so many times, and the number of re-rendering should be less.

PR #6451 reduces such redundant and costly re-rendering in Sidebar.

@ken1kob
Copy link
Contributor Author

ken1kob commented Apr 28, 2022

About inefficient Electron API use

Some of native Electron APIs have a large overhead to issue. During note switching, frequent uses of Electron's MenuBar's APIs in a short term cause a performance problem.

PR #6464 fixes the problem and reduces the time to handle the MenuBar.

@ken1kob
Copy link
Contributor Author

ken1kob commented Apr 28, 2022

About factors proportional to note length

A longer note takes longer time. The followings factors are proportional to note length. And if they become more efficient, the paybacks are greater with note length.

  • (A) Content hashing
  • (B) Conversion from Markdown to Html

However, for (A) and (B), at this point, there are only non-significant improvements. So, no PR will be made in this issue.

@ken1kob
Copy link
Contributor Author

ken1kob commented May 1, 2022

About redundant re-rendering (false positive updates#3)

PR #6469 suppresses redundant re-rendering of NoteEditor caused by ResourceInfos and redundant loading of ResourceInfos.

@ken1kob
Copy link
Contributor Author

ken1kob commented May 1, 2022

About redundant re-rendering (false positive updates#4)

PR #6470 reduces redundant re-rendering in NoteEditor when state.selectedNoteTags is changed but its content is deep equal.

@ken1kob
Copy link
Contributor Author

ken1kob commented May 1, 2022

About redundant re-rendering (false positive dependencies#2)

PR #6471 fixes some false positive dependencies. They are trivial but have not small impacts.

@ken1kob
Copy link
Contributor Author

ken1kob commented May 1, 2022

Performance comparison#2

The below shows comparisons among a version before-improved, a version with two performance bug fix PRs, a version with the two and the previous four improvement PRs, and a version with the more additional five improvement PRs (#6451, #6464, #6469, #6450 and #6451).

Condition Plugins Before
Time (msec)
2 PRs
Time (msec)
2+4 PRs
Time (msec)
2+4+5 PRs
Time (msec)
Total reduced (-%)
Within a notebook no 206 162 136 119 -42%
Within a notebook NoteTabs + Outline 706 219 162 149 -79%
Between notebooks no 1150 688 587 399 -65%
Between notebooks NoteTabs + Outline 1815 821 670 446 -75%

It shows the final performance is about 2-4 times faster than the performance before this issue.

@ken1kob
Copy link
Contributor Author

ken1kob commented May 1, 2022

Result

The below chart shows where the performances of the reference PC (Core i5-4670, CPU Mark=5403, Win10) are in the groups of collected PC performance data (white circles). Simultaneously, it shows where their improved performances (black stars) respectively.

note-switching-time-after

According to the chart, it can be estimated that most PCs will have acceptable performance (less than 1 sec response) by applying the PRs in this issue.

In conclusion, it can be said that the goal of the issue, "For many users' PCs, note switching time becomes less than 1 sec" has become fulfilled.

@github-actions
Copy link
Contributor

Hey there, it looks like there has been no activity on this issue recently. Has the issue been fixed, or does it still require the community's attention? If you require support or are requesting an enhancement or feature then please create a topic on the Joplin forum. This issue may be closed if no further activity occurs. You may comment on the issue and I will leave it open. Thank you for your contributions.

@github-actions github-actions bot added the stale An issue that hasn't been active for a while... label May 31, 2022
@laurent22 laurent22 added enhancement Feature requests and code enhancements desktop All desktop platforms and removed bug It's a bug stale An issue that hasn't been active for a while... labels May 31, 2022
@laurent22
Copy link
Owner

Updated the labels

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
desktop All desktop platforms enhancement Feature requests and code enhancements
Projects
None yet
Development

No branches or pull requests

2 participants