[Discover] Fix performance regression in sidebar #109999

kertal · 2021-08-25T07:31:07Z

Summary

This PR fixes a performance regression in Discover sidebar introduced in 7.14 when searching the sidebar containing lots of fields that need to be grouped (so it's not just the number of fields, but the structure). The function to calculate details displayed in the popover was executed without the popover being displayed. So when 25 fields were displayed, it was calculated 25 times (luckily we just render the fields that are visible to the user). What's more calc_field_count had redundant executions, which also worsens performance.

Here's an example how the performance profile looked like on a system with performance problems:

Testing

I'm adding a files (sidebar-stress.zip) so you can reproduce it. You can use it to provision 500 records with a structure to trigger the performance troubles.

Before that you need to configure the created index so it can contain lot's of fields by running

PUT sidebar-search-test
{
  "settings": {"index.mapping.total_fields.limit": 4000}
}

When ingested, you need to create an index pattern for sidebar-search-test, go to Discover, search in the sidebar, this should no longer be very, very slow.

fixes #109998

Checklist

Unit or functional tests were updated or added to match the most common scenarios

elasticmachine · 2021-08-25T09:05:24Z

Pinging @elastic/kibana-app (Team:KibanaApp)

kertal · 2021-08-25T09:08:14Z

@elasticmachine merge upstream

…performance

rashmivkulkarni · 2021-08-25T20:40:20Z

@kertal - its already past FF for 7.14.1 , but bug fixes are still ok , Do you think it will make it or move the label to 7.14.2 ?

…of github.com:kertal/kibana into kertal-pr-2021-08-25-discover-fix-sidebar-performance

kertal · 2021-08-26T04:34:49Z

@rashmivkulkarni it would be neat to fix this in 7.14.1, I see the tag is not available now, so let's see if I can make it today

kertal · 2021-08-26T05:13:36Z

src/plugins/discover/public/application/apps/main/components/sidebar/discover_field.test.tsx

@@ -15,6 +15,11 @@ import { IndexPatternField } from '../../../../../../../data/public';
 import { stubIndexPattern } from '../../../../../../../data/common/stubs';

 jest.mock('../../../../../kibana_services', () => ({
+  getUiActions: jest.fn(() => {


less error messages when executing the test

kertal · 2021-08-26T06:05:03Z

...ins/discover/public/application/apps/main/components/sidebar/discover_sidebar_responsive.tsx

+  if (fieldCounts.current === null) {
+    fieldCounts.current = calcFieldCounts(
+      {},
+      props.documents$.getValue().result,
+      props.selectedIndexPattern
+    );
+  }


make surecalcFieldCounts is just executed once

I think this can be moved to useEffect, before subscription, which should initialized after mounting

kertal · 2021-08-26T06:12:29Z

src/plugins/discover/public/application/apps/main/components/sidebar/discover_field.tsx

@@ -358,7 +358,36 @@ function DiscoverFieldComponent({
    </EuiPopoverTitle>
  );

-  const details = getDetails(field);
+  const renderPopover = () => {
+    const details = getDetails(field);


making sure getDetails is just executed when needed (popover is displayed)

kibanamachine · 2021-08-26T07:26:15Z

💚 Build Succeeded

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
`discover`	567.4KB	567.5KB	+104.0B

History

💚 Build #148751 succeeded 3c15e62
💚 Build #148481 succeeded bafa77e
💚 Build #148445 succeeded 77f1676

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

cc @kertal

majagrubic · 2021-08-26T08:01:31Z

...ins/discover/public/application/apps/main/components/sidebar/discover_sidebar_responsive.tsx


  const [documentState, setDocumentState] = useState(props.documents$.getValue());
  useEffect(() => {
    const subscription = props.documents$.subscribe((next) => {
      if (next.fetchStatus !== documentState.fetchStatus) {
        if (next.result) {
          fieldCounts.current = calcFieldCounts(
-            next.result.length ? fieldCounts.current : {},
+            next.result.length && fieldCounts.current ? fieldCounts.current : {},


I'm still having trouble understanding this logic. we're calculating fieldCounts, passing in the value that we got as a result of passing in the previous result of calcFieldCounts?

yes, it's like it behaved for a long time, didn't change, but I think it should be in the future.

I believe this logic needs some revisiting, but after testing I don't think it's broken. Let's see if we can do some cleanup here in a follow-up PR.

👍 wanna add an issue for that?

majagrubic

Tested in Chrome on Mac OSX, can confirm that functionality-wise it works same as before. Haven't done a detailed profiling on performance optimization, but it seems pretty clear from the code.

kibanamachine · 2021-08-26T09:32:48Z

💔 Backport failed

Status	Branch	Result
❌	7.15	Commit could not be cherrypicked due to conflicts
✅	7.x
❌	7.14	Commit could not be cherrypicked due to conflicts

Successful backport PRs will be merged automatically after passing CI.

To backport manually run:
node scripts/backport --pr 109999

dimaanj

LGTM, tested in Chrome. One of the thing should be changed in follow up PR here is subscription after mounting.

dimaanj · 2021-08-26T09:33:39Z

...ins/discover/public/application/apps/main/components/sidebar/discover_sidebar_responsive.tsx

+      props.documents$.getValue().result,
+      props.selectedIndexPattern
+    );
+  }

  const [documentState, setDocumentState] = useState(props.documents$.getValue());
  useEffect(() => {
    const subscription = props.documents$.subscribe((next) => {


documentState dependency change leads to initializing subscription two times. So the first listener will not be cleaned up, but I guess it was before.

dimaanj · 2021-08-26T09:34:47Z

...ins/discover/public/application/apps/main/components/sidebar/discover_sidebar_responsive.tsx

+  if (fieldCounts.current === null) {
+    fieldCounts.current = calcFieldCounts(
+      {},
+      props.documents$.getValue().result,
+      props.selectedIndexPattern
+    );
+  }


I think this can be moved to useEffect, before subscription, which should initialized after mounting

Co-authored-by: Matthias Wilhelm <[email protected]>

# Conflicts: # src/plugins/discover/public/application/apps/main/components/sidebar/discover_sidebar_responsive.test.tsx

# Conflicts: # src/plugins/discover/public/application/apps/main/components/sidebar/discover_sidebar_responsive.test.tsx # src/plugins/discover/public/application/apps/main/components/sidebar/discover_sidebar_responsive.tsx

# Conflicts: # src/plugins/discover/public/application/apps/main/components/sidebar/discover_sidebar_responsive.test.tsx

Fix performance regression, part I

77f1676

kertal added Feature:Discover Discover Application release_note:fix Team:Visualizations Visualization editors, elastic-charts and infrastructure v8.0.0 v7.15.0 v7.14.1 labels Aug 25, 2021

kertal self-assigned this Aug 25, 2021

Add test

7ec29cd

kertal marked this pull request as ready for review August 25, 2021 09:05

kertal requested a review from a team August 25, 2021 09:05

Merge branch 'master' into kertal-pr-2021-08-25-discover-fix-sidebar-…

bafa77e

…performance

kertal requested a review from majagrubic August 25, 2021 09:36

kertal added 2 commits August 25, 2021 23:40

Fix redundant useRef calcFieldCounts execution, add test

b0e03a6

Merge branch 'kertal-pr-2021-08-25-discover-fix-sidebar-performance' …

3c15e62

…of github.com:kertal/kibana into kertal-pr-2021-08-25-discover-fix-sidebar-performance

Merge master, fix conflicts

ed8868e

kertal commented Aug 26, 2021

View reviewed changes

kertal added the auto-backport Deprecated - use backport:version if exact versions are needed label Aug 26, 2021

kertal commented Aug 26, 2021

View reviewed changes

majagrubic reviewed Aug 26, 2021

View reviewed changes

majagrubic approved these changes Aug 26, 2021

View reviewed changes

kertal added the v7.16.0 label Aug 26, 2021

kertal merged commit dc07f4d into elastic:master Aug 26, 2021

kibanamachine mentioned this pull request Aug 26, 2021

[7.x] [Discover] Fix performance regression in sidebar (#109999) #110186

Merged

kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Aug 26, 2021

[Discover] Fix performance regression in sidebar (elastic#109999)

7df6a4e

dimaanj reviewed Aug 26, 2021

View reviewed changes

kibanamachine added a commit that referenced this pull request Aug 26, 2021

[Discover] Fix performance regression in sidebar (#109999) (#110186)

8811419

Co-authored-by: Matthias Wilhelm <[email protected]>

kertal added a commit to kertal/kibana that referenced this pull request Aug 26, 2021

[Discover] Fix performance regression in sidebar (elastic#109999)

fdf1ccc

# Conflicts: # src/plugins/discover/public/application/apps/main/components/sidebar/discover_sidebar_responsive.test.tsx

kertal mentioned this pull request Aug 26, 2021

[7.15] [Discover] Fix performance regression in sidebar (#109999) #110216

Merged

kertal mentioned this pull request Aug 26, 2021

[7.14] [Discover] Fix performance regression in sidebar (#109999) #110220

Merged

kertal added a commit that referenced this pull request Aug 26, 2021

[Discover] Fix performance regression in sidebar (#109999) (#110216)

774f8c6

# Conflicts: # src/plugins/discover/public/application/apps/main/components/sidebar/discover_sidebar_responsive.test.tsx

LeeDr added v7.14.2 and removed v7.14.1 labels Aug 31, 2021

kertal added the performance label Sep 9, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Discover] Fix performance regression in sidebar #109999

[Discover] Fix performance regression in sidebar #109999

kertal commented Aug 25, 2021 •

edited

Loading

elasticmachine commented Aug 25, 2021

kertal commented Aug 25, 2021

rashmivkulkarni commented Aug 25, 2021

kertal commented Aug 26, 2021

kertal Aug 26, 2021

kertal Aug 26, 2021 •

edited

Loading

dimaanj Aug 26, 2021

kertal Aug 26, 2021

kibanamachine commented Aug 26, 2021

majagrubic Aug 26, 2021

kertal Aug 26, 2021

majagrubic Aug 26, 2021

kertal Aug 26, 2021

majagrubic left a comment

kibanamachine commented Aug 26, 2021

dimaanj left a comment

dimaanj Aug 26, 2021

dimaanj Aug 26, 2021

[Discover] Fix performance regression in sidebar #109999

[Discover] Fix performance regression in sidebar #109999

Conversation

kertal commented Aug 25, 2021 • edited Loading

Summary

Testing

Checklist

elasticmachine commented Aug 25, 2021

kertal commented Aug 25, 2021

rashmivkulkarni commented Aug 25, 2021

kertal commented Aug 26, 2021

Choose a reason for hiding this comment

kertal Aug 26, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kibanamachine commented Aug 26, 2021

💚 Build Succeeded

Metrics [docs]

Async chunks

History

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

majagrubic left a comment

Choose a reason for hiding this comment

kibanamachine commented Aug 26, 2021

💔 Backport failed

dimaanj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kertal commented Aug 25, 2021 •

edited

Loading

kertal Aug 26, 2021 •

edited

Loading