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

Fix: Defer re-decorating note when changing search #2073

Merged
merged 3 commits into from
May 12, 2020

Conversation

dmsnell
Copy link
Member

@dmsnell dmsnell commented May 11, 2020

See #1941
See #1982

When we rebuilt the search to return instant results and removed the debounce
on the search filter we exposed an issue with note rendering performance that
ironically made the new instant search slower than the old one in certain
circumstances, namely when a note in the search results takes a very long time
to render.

The leading reason for the performance issue is that draft-js was applying
a new decorator to its note on every change to the search field. With the
search field updating instantly that left no time for the decorators to redraw
(and they were very inefficient to make it worse).

In this patch we're delaying the re-decoration until the search field settles.
This doesn't eliminate the problem but it should bring it roughly on par with
the behavior from before the search updates. Further we have eliminated the
MultiDecorator dependency since that functionality is provided by draft-js
itself
Scratch that - there's a slight API change that would require more code to change. Since there's no need to create a composite decorator when the search
query doesn't contain any text terms we can futher skip it and only decorate
with the checkbox decorator.

These changes should make searching tolerant to slow notes and should
additionally cut the time it takes to decorate notes approximately in half.

Changes

  • delays highlighting search results inside of the open note when making search changes or when switching notes. the delay is currently set at 500ms and only affects notes whose contents are longer than 10,000 code units

Notes

  • Long notes may still render slowly due to the cost of initializing draft-js. Further work to improve this probably needs to involve replacing draft-js

@dmsnell dmsnell requested review from codebykat and belcherj May 11, 2020 22:30
@dmsnell dmsnell force-pushed the fix/delay-re-decoration branch from a199503 to afe6040 Compare May 11, 2020 22:31
Copy link
Contributor

@belcherj belcherj left a comment

Choose a reason for hiding this comment

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

Good to go. Tested and works well.

dmsnell added 3 commits May 12, 2020 14:53
See #1941
See #1982

When we rebuilt the search to return instant results and removed the debounce
on the search filter we exposed an issue with note rendering performance that
ironically made the new instant search slower than the old one in certain
circumstances, namely when a note in the search results takes a very long time
to render.

The leading reason for the performance issue is that `draft-js` was applying
a new decorator to its note on every change to the search field. With the
search field updating instantly that left no time for the decorators to redraw
(and they were very inefficient to make it worse).

In this patch we're delaying the re-decoration until the search field settles.
This doesn't eliminate the problem but it should bring it roughly on par with
the behavior from before the search updates. Further we have eliminated the
`MultiDecorator` dependency since that functionality is provided by `draft-js`
itself. Since there's no need to create a composite decorator when the search
query doesn't contain any text terms we can futher skip it and only decorate
with the checkbox decorator.

These changes should make searching tolerant to slow notes and should
additionally cut the time it takes to decorate notes approximately in half.
@belcherj belcherj force-pushed the fix/delay-re-decoration branch from 1f1d636 to 7f397b9 Compare May 12, 2020 18:54
@belcherj belcherj merged commit 3308f20 into develop May 12, 2020
@belcherj belcherj deleted the fix/delay-re-decoration branch May 12, 2020 20:43
@codebykat codebykat added this to the 1.17 milestone May 18, 2020
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.

3 participants