-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[SearchBar] Improve rendering performance #119189
[SearchBar] Improve rendering performance #119189
Conversation
@elasticmachine merge upstream |
7944f9c
to
4f0ee14
Compare
…timize-search-bar-react-rendering
@@ -286,7 +286,7 @@ | |||
"markdown-it": "^10.0.0", | |||
"md5": "^2.1.0", | |||
"mdast-util-to-hast": "10.0.1", | |||
"memoize-one": "^5.0.0", | |||
"memoize-one": "^6.0.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updating because now it has clear()
method
import { shallowEqual } from '../../utils/shallow_equal'; | ||
|
||
const SuperDatePicker = React.memo( | ||
EuiSuperDatePicker as any |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couldn't find a way to avoid any
here
); | ||
}); | ||
|
||
function useDimensions( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is used for positioning suggestions component relative to the input field. Previously it was scattered between parent and child, now it is centralized inside a child.
This allowed to avoid trigger parent's re-rendering on page resizes and avoid reduce dimensions reading that cause layout trashing
@elasticmachine merge upstream |
Pinging @elastic/kibana-app-services (Team:AppServicesUx) |
@elasticmachine merge upstream |
…timize-search-bar-react-rendering # Conflicts: # src/plugins/data/public/ui/query_string_input/query_string_input.tsx
…thub.com:Dosant/kibana into d/2021-11-15-optimize-search-bar-react-rendering
@elasticmachine merge upstream |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work @Dosant ! I tested locally and with react profiler I observed fewer peaks in re-renders as I was interacting with the search bar and suggestions 👏🏻
I left a few ideas on how we could improve some minor points.
} | ||
} | ||
|
||
if (document.activeElement !== null && document.activeElement.id === this.textareaId) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (document.activeElement !== null && document.activeElement.id === this.textareaId) { | |
if (document.activeElement?.id === this.textareaId) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO this reads a lot easier, probably has a negligible performance overhead. I'll leave it up to you :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that is easier to read, but logically this is a bit different. undefined
=== undefined
case is different.
I'd leave it as is, because need to review this logical case first to make this change
window.addEventListener('scroll', handler, { passive: true, capture: true }); | ||
|
||
const resizeObserver = | ||
typeof window.ResizeObserver !== 'undefined' && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can go without this check as it looks fairly supported in major browsers https://caniuse.com/resizeobserver.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree that support is pretty good, but I was actually deciding between:
A. Using it only when available because this isn't critical functionality (so if it is not available, everything is still working, but with some position bugs)
B. Using resize-observer-polyfill
like we do in other places. But I wanted to avoid including couple more Kbs into this async chunk for this - https://bundlephobia.com/package/resize-observer-polyfill@1.5.1
C. Using it directly assuming it always exists. I didn't go with it because it wasn't necessary to introduce this breaking change. As it might break someone who is using Kibana on some TV platform or older Safari without a good need for it. But since we are actually using it in some places directly, I created: #121509 to address this separately
|
||
return () => { | ||
window.removeEventListener('scroll', handler, { capture: true }); | ||
if (resizeObserver) resizeObserver.disconnect(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (resizeObserver) resizeObserver.disconnect(); | |
resizeObserver?.disconnect(); |
Or just resizeObserver.disconnect();
if we remove the conditional
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is currently written so that resizeObserver is false | ResizeObserver
so this doesn't work
@elasticmachine merge upstream |
…timize-search-bar-react-rendering # Conflicts: # src/plugins/data/public/ui/query_string_input/query_bar_top_row.tsx # src/plugins/data/public/ui/search_bar/search_bar.tsx
@elasticmachine merge upstream |
@elasticmachine merge upstream |
@elasticmachine merge upstream |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested, functionality seems to be working properly.
|
||
if ( | ||
(newSelectionStart !== null && this.inputRef?.selectionStart !== newSelectionStart) || | ||
(newSelectionEnd !== null && this.inputRef?.selectionEnd !== newSelectionEnd) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When can newSelectionStart
be null
? And is this a case that should actually still be calling setState
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, should we add these same checks above for query
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When can newSelectionStart be null? And is this a case that should actually still be calling setState?
Don't remember why I put it, I re-checeked and indeed doesn't seem necessary. Removed
Also, should we add these same checks above for query?
There is important check regarding the query inside onQueryStringChange
Just a note: I didn't try to put equal checks before setState everywhere, I was looking at render profile and added this checks why I saw it makes an impact
…timize-search-bar-react-rendering
@elasticmachine merge upstream |
@elasticmachine merge upstream |
💚 Build Succeeded
Metrics [docs]Module Count
Public APIs missing comments
Async chunks
Page load bundle
History
To update your PR or re-run it, just comment with: |
Summary
This pr is set of small UI performance improvements for our SearchBar component.
This is the result of my space time where I explored different react performance improvements techniques. doc
Some of the changes:
PureComponent
) where it makes sense. For example, we don't need to re-run a render cycle on<FilterBar/>
when user types intoQueryInput
setState
and changing the object ref when it is not needed to avoid re-renderinguseCallback
or move them as class method to get a stable referenceuseMemo
to get a stable reference<SharingMetaFields />
)render
. (see layout thrashing). For this had to refactor position tracking for suggestions.requestAnimationFrame
scheduler. This is especially beneficial for those that read layout values from DOM elements. (e.g.scrollIntoView
in<SuggestionsComponent/>
)Risk Matrix
filters
aggregation.How I tested performance
For my sample test results are: before ~150ms, after ~70ms.
I used react profiler API to sum up rendering time: https://reactjs.org/docs/profiler.html
And my sample test was typing the query using typeahead:
Sample test