Skip to content

Conversation

@mbondyra
Copy link
Contributor

@mbondyra mbondyra commented Feb 17, 2021

Summary

Involves performance changes:

  • pass context directly to buttons and not to the whole layer panel to avoid rerendering when context changes
  • only pass dragging to getDropProps and don't pass it at all to onDrop (we didn't use it at all)
  • memoized some values passed to DragDrop
  • memorized ChartSwitch as it was rerendering everytime when workspace is rerendered (so any dragDropContext change)
  • memoized suggestionForDraggedField as this function is quite expensive (2~8ms) and was being executed twice on changing the context when dragging started
  • moved changeIndexPattern up so it's not being recreated on every render.
  • Unit or functional tests were updated or added to match the most common scenarios
  • flattening structure of drag drop Context - before activeDropTarget consisted of activeDropTarget and dropTargetsByOrder so everytime we registered dropTargetsByOrder all the elements rerendered. Now activeDropTarget and dropTargetsByOrder are two separate props of a context.
  • adding the style below, to avoid the flickering of dragOver-dragEnd that currently is happening when dragging around the drop:
.lnsDragDrop-isDropTarget {
  > * {
    pointer-events: none;
  }
}

before:
bug
after:
after

  • only registering droptargetsByOrder when in keyboardMode - thanks to that context changes only once and not two times when starting a drag by mouse. So we have one rerender instead of two for all the datapanel fields (in the picture you can see 4 & 2 rerenders, that's because I track dragStart (2 rerenders on the picture 'before' and 1 'after') and dragEnd (2 rerenders before and 1 after) as I cannot stop profiler without stopping a drag.

Results

Before:

After:

Example improvement in editor frame: When starting a drag, we have two rerenders - when dragging changes in context, and then when all the dropTargetsByOrder register. The second rerender improved a lot, the first not so much as the dragging props has to be passed down to many components.

before when starting a drag, illustrated 2 rerenders :

after:

The message below disappeared when starting mouse dragging:
image

When throttling my processor x 4 times slower, the message still appears but times dropped by more than a half and don't appear for dragend at all:

before (repeatedly starting a drag and dropping in empty area):
image

after:

I have some ideas to further improve performance, but I don't want to do this ahead of time as big changes are awaiting so I'll leave it as it is.

@mbondyra mbondyra added Team:Visualizations Team label for Lens, elastic-charts, Graph, legacy editors (TSVB, Visualize, Timelion) t// v8.0.0 release_note:skip Skip the PR/issue when compiling release notes Feature:Lens v7.12.0 labels Feb 17, 2021
@mbondyra mbondyra requested a review from a team February 17, 2021 13:42
@elasticmachine
Copy link
Contributor

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

@mbondyra mbondyra changed the title Lens/perf dnd [Lens] Drag and drop performance improvements Feb 17, 2021
@mbondyra mbondyra changed the title [Lens] Drag and drop performance improvements [Lens] Drag and drop performance improvements p1 Feb 17, 2021
@mbondyra mbondyra changed the title [Lens] Drag and drop performance improvements p1 [Lens] Drag and drop performance improvements Feb 17, 2021
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this change is only removing ChildDragDropProvider and removing dragDropContext from buttons' props.

@dej611
Copy link
Contributor

dej611 commented Feb 18, 2021

Tested on Chrome and profiled it. There's a good perf boost from master.
I've commented a couple of places where I think it can be improved.

I'm a bit unsure whether all the memoization in x-pack/plugins/lens/public/editor_frame_service/editor_frame/workspace_panel/workspace_panel.tsx is required. For instance things like renderEmptyWorkspace can work ok even without it.

@mbondyra
Copy link
Contributor Author

mbondyra commented Feb 19, 2021

Hey @dej611! Thank you for checking my PR! About the memoizations in the workspace_panel component, you’re right – I firstly made it this way because it made sense, and then I switched to moving context in upper component, so it doesn’t matter that much – I don’t have a strong opinion about leaving it or moving it there so I'll just remove it, please take a look again.

Copy link
Contributor

@MichaelMarcialis MichaelMarcialis left a comment

Choose a reason for hiding this comment

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

LGTM

@mbondyra
Copy link
Contributor Author

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
lens 482 485 +3

Async chunks

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

id before after diff
lens 908.1KB 918.2KB +10.1KB

History

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

@mbondyra mbondyra merged commit 65be9e0 into elastic:master Feb 21, 2021
mbondyra added a commit to mbondyra/kibana that referenced this pull request Feb 21, 2021
mbondyra added a commit to mbondyra/kibana that referenced this pull request Feb 21, 2021
jloleysens added a commit to jloleysens/kibana that referenced this pull request Feb 22, 2021
…ndition-for-hiding-recommded-allocation

* 'master' of github.com:elastic/kibana: (117 commits)
  [coverage] ingest data in parallel (elastic#92074)
  [Lens] Drag and drop performance improvements (elastic#91641)
  A few more environment uiFilters fixes (elastic#92044)
  Enabling Uptime and Dashboard a11y test (elastic#91017)
  [Security Solution][Detections] Adds more granular validation for nested fields (elastic#92041)
  [Security Solution] [Detections] add overflow-wrap for description (elastic#91945)
  [Security Solution] [Detections] do not truncate filename in value list table in modal (elastic#91952)
  Skip flaky apm test elastic#91673 (elastic#92065)
  [docker] Default server.name to hostname (elastic#90799)
  Use documentation link service for snapshot restore (elastic#91596)
  [Security Solution] Clearing up all jest errors and warnings (elastic#91740)
  Add `@kbn/analytics` to UI Shared Deps (elastic#91810)
  [7.12][Telemetry] Add missing fields for security telemetry (elastic#91920)
  [Security Solution] Adds cypress-pipe (elastic#91550)
  [ML] Fix event rate chart annotation position (elastic#91899)
  [APM] Break down error table api removing the sparklines (elastic#89138)
  docs: update dependencies table bug (elastic#91964)
  [Time to Visualize] Stay in Edit Mode After Dashboard Quicksave (elastic#91729)
  Unskip Search Sessions Management UI test (elastic#90110)
  [Fleet] Handle long text in agent details page (elastic#91776)
  ...

# Conflicts:
#	x-pack/plugins/index_lifecycle_management/__jest__/client_integration/edit_policy/edit_policy.helpers.tsx
#	x-pack/plugins/index_lifecycle_management/__jest__/components/edit_policy.test.tsx
gmmorris added a commit to gmmorris/kibana that referenced this pull request Feb 22, 2021
…bana into task-manager/docs-monitoring

* 'task-manager/docs-monitoring' of github.com:gmmorris/kibana: (29 commits)
  Update docs/developer/plugin-list.asciidoc
  Update docs/api/task-manager/health.asciidoc
  Update docs/api/task-manager/health.asciidoc
  [Lens] Load indexpatterns list from indexPattern Service (elastic#91984)
  [coverage] ingest data in parallel (elastic#92074)
  [Lens] Drag and drop performance improvements (elastic#91641)
  A few more environment uiFilters fixes (elastic#92044)
  Enabling Uptime and Dashboard a11y test (elastic#91017)
  [Security Solution][Detections] Adds more granular validation for nested fields (elastic#92041)
  [Security Solution] [Detections] add overflow-wrap for description (elastic#91945)
  [Security Solution] [Detections] do not truncate filename in value list table in modal (elastic#91952)
  Skip flaky apm test elastic#91673 (elastic#92065)
  [docker] Default server.name to hostname (elastic#90799)
  Use documentation link service for snapshot restore (elastic#91596)
  [Security Solution] Clearing up all jest errors and warnings (elastic#91740)
  Add `@kbn/analytics` to UI Shared Deps (elastic#91810)
  [7.12][Telemetry] Add missing fields for security telemetry (elastic#91920)
  [Security Solution] Adds cypress-pipe (elastic#91550)
  [ML] Fix event rate chart annotation position (elastic#91899)
  [APM] Break down error table api removing the sparklines (elastic#89138)
  ...
gmmorris added a commit to gmmorris/kibana that referenced this pull request Feb 22, 2021
* master: (36 commits)
  [Uptime] Thumbnail full screen view steps navigation fix (elastic#91895)
  Implement ScopedHistory.block (elastic#91099)
  [Lens] Fix overlowing content on a chart for charts and table (elastic#92006)
  handle source column differences in embeddable as well (elastic#91987)
  [Vega] [Map] disable map rotation using right right click /  touch rotation gesture (elastic#91996)
  [Lens] Load indexpatterns list from indexPattern Service (elastic#91984)
  [coverage] ingest data in parallel (elastic#92074)
  [Lens] Drag and drop performance improvements (elastic#91641)
  A few more environment uiFilters fixes (elastic#92044)
  Enabling Uptime and Dashboard a11y test (elastic#91017)
  [Security Solution][Detections] Adds more granular validation for nested fields (elastic#92041)
  [Security Solution] [Detections] add overflow-wrap for description (elastic#91945)
  [Security Solution] [Detections] do not truncate filename in value list table in modal (elastic#91952)
  Skip flaky apm test elastic#91673 (elastic#92065)
  [docker] Default server.name to hostname (elastic#90799)
  Use documentation link service for snapshot restore (elastic#91596)
  [Security Solution] Clearing up all jest errors and warnings (elastic#91740)
  Add `@kbn/analytics` to UI Shared Deps (elastic#91810)
  [7.12][Telemetry] Add missing fields for security telemetry (elastic#91920)
  [Security Solution] Adds cypress-pipe (elastic#91550)
  ...
@mbondyra mbondyra deleted the lens/perf-dnd branch July 6, 2023 12:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Feature:Lens 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// v7.12.0 v7.13.0 v8.0.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants