[Lens] Performance refactoring for indexpattern fast lookup and Operation support matrix computation by dej611 · Pull Request #82829 · elastic/kibana

dej611 · 2020-11-06T13:43:41Z

Summary

Due to new validation checks being added to the Lens UI while the user performs actions (see #81439 and #82607 ) some performance tweaks have been performed to speed up lookup operations for indexpattern fields and related operations.

✨ Add new getFieldByName method to the Lens' Indexpattern interface implementing a fast lookup mechanism
- propagate the use of this new API for any indexpattern.fields.find() instance
- replace some local lookup instances with the new API
- reshape some components to accept the new API
⚡ Refactor the getOperationSupportMatrix to be fast
⚡ Reshaping getOperationSupportMatrix to use Sets instead of array for faster lookups

Fixes #82243

Before	After

the bigger contribution of performance comes from the refactor of getOperationSupportMatrix which is now taking at least 80% less time:

Before	After

Checklist

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

dej611 · 2020-11-06T13:53:45Z

Profile before

Profile after

dej611 · 2020-11-06T14:31:47Z

The profiling and measurements above have been tested on an IndexPattern of 4000+ fields.

…xpattern-fields-lookup

dej611 · 2020-11-06T16:30:02Z

Looking also at this portion of code, which seems to waste the conditional task away: https://github.com/elastic/kibana/pull/82829/files#diff-adf4f7cb4d70d7250d44841daef76d1c7a4794a06f7ca261510e1b1112e18bb4R283-R308

Maybe there's a missing else there?

flash1293 · 2020-11-06T16:37:40Z

I looked into the changes and it seems like the large performance bottleneck in the support operation matrix got introduced very recently here: f0023ed

elasticmachine · 2020-11-09T08:52:47Z

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

dej611 · 2020-11-09T08:53:05Z

@elasticmachine merge upstream

flash1293

Looks pretty good, left some comments but nothing major

x-pack/plugins/lens/public/indexpattern_datasource/dimension_panel/bucket_nesting_editor.tsx

flash1293 · 2020-11-09T14:37:15Z

x-pack/plugins/lens/public/indexpattern_datasource/dimension_panel/field_select.tsx

    function fieldNamesToOptions(items: string[]) {
      return items
        .map((field) => ({
-          label: fieldMap[field].displayName,


This doesn't seem like a good handling of the error case here (it could create entries without a label) - let's filter out all fields without entry in the field map instead before mapping

How would that happen? Fields are validated against the containsData here

They are not for all invocations - fieldNamesToOptions is also called for empty fields in line 138 of this file.

flash1293 · 2020-11-09T14:37:40Z

x-pack/plugins/lens/public/indexpattern_datasource/dimension_panel/operation_support.ts


  filteredOperationsByMetadata.forEach(({ operations }) => {
    operations.forEach((operation) => {
+      // replace spread operator by regular push as it's performance wise faster


Nit: Let's remove this comment, it won't make sense after the PR is merged.

The idea was to keep it to prevent future use of spread operator again

I see, got confused by the wording. Can we go with // using mutating push instead of spread operator as it's faster ?

flash1293 · 2020-11-09T14:39:05Z

x-pack/plugins/lens/public/indexpattern_datasource/dimension_panel/operation_support.ts

    operations.forEach((operation) => {
+      // replace spread operator by regular push as it's performance wise faster
      if (operation.type === 'field') {
-        supportedOperationsByField[operation.field] = [


@wylieconlon Replacing push with a spread here happened relatively recently in a refactor PR you did (splitting up the dimension editor). Is there any reason for preferring spreading?

x-pack/plugins/lens/public/indexpattern_datasource/indexpattern_suggestions.ts

flash1293 · 2020-11-09T14:46:22Z

x-pack/plugins/lens/public/indexpattern_datasource/operations/operations.ts


  operationDefinitions.sort(getSortScoreByPriority).forEach((operationDefinition) => {
    if (operationDefinition.input === 'field') {
-      indexPattern.fields.forEach((field) => {


I don't see how these changes improve the code - it's still nested to the same depth (plus addToMap does not actually return something). Can you elaborate?

Not a particular speed up here.
I did some small profiling and numbers are bit lower than with all the conditionals: seeing no harm in particular change I kept it, but it can easily reversed.

with all the conditionals

Which conditionals do you mean? I wasn't aware this was a performance optimization, can you explain how it could speed things up? I think I'm missing something here

I meant "from the previous implementations with the if/else conditionals".
It may be an anecdotal to my machine, so no strong opinion on these.

…dej611/kibana into feature/lens/indexpattern-fields-lookup

mbondyra · 2020-11-10T10:28:41Z

@elasticmachine merge upstream

mbondyra · 2020-11-10T10:54:07Z

Tested in Chrome, I didn't find any problems with it.

flash1293

Changes LGTM, didn't test again. Thanks for addressing all the points!

kibanamachine · 2020-11-10T12:13:21Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: c5c623b

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
`lens`	1.0MB	1.0MB	+975.0B

History

💚 Build #86881 succeeded 3f4fde5
💚 Build #86656 succeeded e4709eb
💚 Build #86436 succeeded f5a26a5
💚 Build #86403 succeeded 52cd945
💔 Build #86391 failed c2bd377

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

…tion support matrix computation (elastic#82829) Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

…kibana into bootstrap-node-details-overlay * 'bootstrap-node-details-overlay' of github.com:phillipb/kibana: (49 commits) [Security Solution] Fix DNS Network table query (elastic#82778) [Workplace Search] Consolidate groups routes (elastic#83015) Adds cloud links to user menu (elastic#82803) [Security Solution][Detections] - follow up cleanup on auto refresh rules (elastic#83023) [App Search] Added the log retention panel to the Settings page (elastic#82982) [Maps] show icon when layer is filtered by time and allow layers to ignore global time range (elastic#83006) [DOCS] Consolidates drilldown pages (elastic#82081) [Maps] add on-prem EMS config (elastic#82525) migrate i18n mixin to KP (elastic#81799) [bundle optimization] fix imports of react-use lib (elastic#82847) [Discover] Add metric on adding filter (elastic#82961) [Lens] Performance refactoring for indexpattern fast lookup and Operation support matrix computation (elastic#82829) skip flaky suite (elastic#82804) Fix SO query for searching across spaces (elastic#83025) renaming built-in alerts to Stack Alerts (elastic#82873) [TSVB] Disable using top_hits in pipeline aggregations (elastic#82278) [Visualizations] Remove kui usage (elastic#82810) [Visualizations] Make the icon buttons labels more descriptive (elastic#82585) [Lens] Do not reset formatting when switching between custom ranges and auto histogram (elastic#82694) Fix ilm navigation (elastic#81664) ...

… Operation support matrix computation (#82829) (#83066) Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

…na into alerts/stack-alerts-public * 'alerts/stack-alerts-public' of github.com:gmmorris/kibana: [Security Solution] Fix DNS Network table query (elastic#82778) [Workplace Search] Consolidate groups routes (elastic#83015) Adds cloud links to user menu (elastic#82803) [Security Solution][Detections] - follow up cleanup on auto refresh rules (elastic#83023) [App Search] Added the log retention panel to the Settings page (elastic#82982) [Maps] show icon when layer is filtered by time and allow layers to ignore global time range (elastic#83006) [DOCS] Consolidates drilldown pages (elastic#82081) [Maps] add on-prem EMS config (elastic#82525) migrate i18n mixin to KP (elastic#81799) [bundle optimization] fix imports of react-use lib (elastic#82847) [Discover] Add metric on adding filter (elastic#82961) [Lens] Performance refactoring for indexpattern fast lookup and Operation support matrix computation (elastic#82829) skip flaky suite (elastic#82804) Fix SO query for searching across spaces (elastic#83025) renaming built-in alerts to Stack Alerts (elastic#82873) [TSVB] Disable using top_hits in pipeline aggregations (elastic#82278) [Visualizations] Remove kui usage (elastic#82810) [Visualizations] Make the icon buttons labels more descriptive (elastic#82585) [Lens] Do not reset formatting when switching between custom ranges and auto histogram (elastic#82694) :

dej611 added 5 commits November 5, 2020 17:35

⚡ Use a lookup for indexPattern fields

1e2b3d0

✅ Update all tests to handle new lookup format

03dd62c

✨ Migrate from map to method

7b0ac89

✨ Expand the use of the getFieldByName method

f586105

⚡ Improve getOperationSupportMatrix performance

c2bd377

🏷️ Fix type issue

52cd945

dej611 added Feature:Lens v7.11.0 v8.0.0 labels Nov 6, 2020

Merge remote-tracking branch 'upstream/master' into feature/lens/inde…

f5a26a5

…xpattern-fields-lookup

dej611 added release_note:skip Skip the PR/issue when compiling release notes Team:Visualizations Team label for Lens, elastic-charts, Graph, legacy editors (TSVB, Visualize, Timelion) t// labels Nov 6, 2020

dej611 marked this pull request as ready for review November 9, 2020 08:52

dej611 requested a review from a team November 9, 2020 08:52

Merge branch 'master' into feature/lens/indexpattern-fields-lookup

e4709eb

flash1293 reviewed Nov 9, 2020

View reviewed changes

dej611 added 5 commits November 9, 2020 17:21

👌 Implement feedbacks

39fc23d

Merge branch 'feature/lens/indexpattern-fields-lookup' of github.com:…

7815ba0

…dej611/kibana into feature/lens/indexpattern-fields-lookup

♻️ Restore old branching

f9b7bec

✏️ Forgotten to remove one return

881e95e

⚡ Migrate to Set from array to speed up dnd lookups

3f4fde5

Merge branch 'master' into feature/lens/indexpattern-fields-lookup

c5c623b

mbondyra approved these changes Nov 10, 2020

View reviewed changes

flash1293 approved these changes Nov 10, 2020

View reviewed changes

dej611 merged commit 0b99841 into elastic:master Nov 10, 2020

dej611 deleted the feature/lens/indexpattern-fields-lookup branch November 10, 2020 13:31

dej611 mentioned this pull request Nov 10, 2020

[7.x] [Lens] Performance refactoring for indexpattern fast lookup and Operation support matrix computation (#82829) #83066

Merged

Conversation

dej611 commented Nov 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Checklist

Uh oh!

dej611 commented Nov 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Profile before

Profile after

Uh oh!

dej611 commented Nov 6, 2020

Uh oh!

dej611 commented Nov 6, 2020

Uh oh!

flash1293 commented Nov 6, 2020

Uh oh!

elasticmachine commented Nov 9, 2020

Uh oh!

dej611 commented Nov 9, 2020

Uh oh!

flash1293 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mbondyra commented Nov 10, 2020

Uh oh!

mbondyra commented Nov 10, 2020

Uh oh!

flash1293 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kibanamachine commented Nov 10, 2020

💚 Build Succeeded

Metrics [docs]

Async chunks

History

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

dej611 commented Nov 6, 2020 •

edited

Loading

dej611 commented Nov 6, 2020 •

edited

Loading

flash1293 left a comment •

edited

Loading