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

perf(tooltip): improve placement logic #2310

Merged
merged 3 commits into from
Jan 29, 2024

Conversation

nickofthyme
Copy link
Collaborator

@nickofthyme nickofthyme commented Jan 23, 2024

Summary

Fixes performance issue notice with synced tooltips.

Details

The popperSettings value here...

const popperSettings = useMemo((): TooltipPortalSettings | undefined => {
if (!settings || typeof settings === 'string') {
return;
}
const { placement, fallbackPlacements, boundary, ...rest } = settings;
return {
...rest,
placement: placement ?? (rotation === 0 || rotation === 180 ? Placement.Right : Placement.Top),
fallbackPlacements:
fallbackPlacements ??
(rotation === 0 || rotation === 180
? [Placement.Right, Placement.Left, Placement.Top, Placement.Bottom]
: [Placement.Top, Placement.Bottom, Placement.Right, Placement.Left]),
boundary: boundary === 'chart' ? chartRef.current ?? undefined : boundary,
};
}, [settings, chartRef, rotation]);

...was triggering an update for every new render even though the settings values had not changed.

This was caused by the getTooltipSettings helper method here...

function getTooltipSettings(
tooltip: TooltipSpec,
{ externalPointerEvents }: SettingsSpec,
isExternalTooltipVisible: boolean,
): TooltipProps {
if (!isExternalTooltipVisible) return tooltip;
return {
...tooltip,
...externalPointerEvents.tooltip,
};
}

...which was returning a new merged instance of the settings for every external event.

Issues

fixes #2309

Checklist

  • The proper chart type label has been added (e.g. :xy, :partition)
  • The proper feature labels have been added (e.g. :interactions, :axis)
  • All related issues have been linked (i.e. closes #123, fixes #123)
  • Unit tests have been added or updated to match the most common scenarios

@nickofthyme nickofthyme added :tooltip Related to hover tooltip :all Applies to all chart types labels Jan 23, 2024
@nickofthyme nickofthyme changed the title fix(tooltip): eager popperjs creation perf(tooltip): improve placement logic Jan 26, 2024
@nickofthyme nickofthyme merged commit cac5f49 into elastic:main Jan 29, 2024
13 checks passed
@nickofthyme nickofthyme deleted the fix-popper branch January 29, 2024 17:35
nickofthyme pushed a commit that referenced this pull request Jan 29, 2024
# [63.1.0](v63.0.0...v63.1.0) (2024-01-29)

### Bug Fixes

* **deps:** update dependency @elastic/eui to ^92.1.1 ([#2315](#2315)) ([f4e4fae](f4e4fae))
* **deps:** update dependency @playwright/test to ^1.41.1 ([#2316](#2316)) ([e2ab527](e2ab527))
* **styles:** isolated point style overrides ([#2278](#2278)) ([3fb1df2](3fb1df2))

### Features

* **metric:** custom slot to render contents in gap ([#2303](#2303)) ([3256c8c](3256c8c))

### Performance Improvements

* **tooltip:** improve placement logic ([#2310](#2310)) ([cac5f49](cac5f49))
markov00 added a commit to elastic/kibana that referenced this pull request May 16, 2024
## Summary

This quick fix reduces the debounce time to 8 milliseconds (from 40
millis) for synchronized cursors in dashboards.
This results in snappier synchronized cursors in dashboard with an
improved perceived performance.

This change can be done safely due to the improvements in the chart
Tooltip implementation in
elastic/elastic-charts#2310.

Fix [#176609](#176609)


https://github.com/elastic/kibana/assets/1421091/599d4157-3f75-41a9-b838-4eb40938dcaa

I've tested with 4x and 6x CPU slowdown and I don't see any major
differences between the current main (with 40ms debounce) and this
version with small debounce: both have similar lag.

To test this, create a dashboard with multiple TSVB charts and other
Timeseries Lens charts and move the cursor across them. The synchronized
cursor should move smoothly across each chart

---------

Co-authored-by: Nick Partridge <nicholas.partridge@elastic.co>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:all Applies to all chart types :tooltip Related to hover tooltip
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Popperjs is excessively creating new instances for external tooltips
2 participants