Performance issues on a large number of poppers

[hmaurer]

Hello,

I am running into rendering performance issues while using react-popper through BlueprintJS (their tooltip component is backed by react-popper). When using a large number (365) of tooltips (aka a large number of react-popper manager + target combination), the initial render is very slow (hundreds of milliseconds).

I will create a jsfiddle to attempt to reproduce the issue, but I wanted to open the issue already because it is quite a big block on the project I am working on and I was wondering if this is fully expected behaviour or a bug that needs to be investigated (or perhaps even an odd interaction with another library I am using, making the problem independent from react-popper).

Thanks!

[giladgray]

@hmaurer try setting usePortal={false} on your tooltips. that will hopefully have a significant effect on performance.

[krizpoon]

Recently I encountered the same issue. I was using Material-UI v1.2.1 Tooltip component on a table with about 300 cells and each cell having 4 tooltips. It took 9 seconds to render in Safari (in Chrome it's only fraction of a second though). My solution is to add a mouse hover timer and only render the tooltip if mouse cursor is over for 500ms. I don't need the tooltip to show up immediately so it's ok. The table renders instantly.

[rosskevin]

Material-ui just reverted to pre-1.0 react-popper release due to this performance concern.

mui/material-ui#11920
mui/material-ui#11913

[oliviertassinari]

@rosskevin I'm removing react-popper from Material-UI. The v1.0.0 leak regression was a good opportunity for me to dive into the internals of react-popper. I disagree with some of the tradeoffs that have been taken here. For instance, the usage of the context increases the bundle size, introduce a lot of rerendering for keeping all the states up to date, makes tests harder with enzyme, makes the logic brittle (leak), etc.

[email protected] will come with a new <Popper /> component: an abstraction on top of Popper.js with very few internal dependencies on Material-UI core. The API is almost the same one as the Popover but answer different needs. This new components will be used internally by the <Tooltip />.

https://material-ui.com/utils/popper/

[jquense]

We are planning on using react-popper for the v4 bootstrap support in react-bootstrap. I'd be happy to help out with this issue, since it'll likely affect our users as well. The "size" here doesn't bother us, react-create-context is not a long term dep (and apps are already likely to contain it) and otherwise everything is super minimal. The API is 👍 and nice and forward thinking. Great work everyone

I'm not the best at doing perf profiling or analysis but i'd be happy to do the grunt work making changes if someone would be willing to work with me to identify where the potential problem areas are.

I will say tho it's not clear to me that react-popper is even the perf bottleneck at 500+ tooltips

[oliviertassinari]

@jquense I'm very grateful to all the work that was done! I will keep a close look at how things evolves here :). The more paths are explored the better choice you can try to make. My core issue isn't with react-create-context. It's structural. It's easy to use Popper.js nacked, I'm having a hard time seeing the need for a direct React wrapper, or something with more than 50 lines. On Material-UI, the wrapper is ~250 lines, I try to cover more use cases: rtl, portal and transitions. Anyway, I love this way of thinking:

Perfection is achieved, not when there is nothing more to add, but when there is nothing left to take away.

Antoine de Saint-Exupery.

Keep up the good work!

For reference:

• https://github.com/reactstrap/reactstrap/blob/2d75db09c382124529c9b5c38e70b8791875ee5b/package.json#L104
• https://github.com/palantir/blueprint/blob/c133b0b6f7f7c1ca0ec0931b6aa6a40098faaaec/packages/core/package.json#L47

[FezVrasta]

The main reason for the existence of this wrapper is to delegate all the DOM manipulations to React. You can save quite some lines of code if you strip out that part but the final result will not play as nicely

[jquense]

It should also be said that react-popper is only like 350 loc to begin with, it's not really a high cost dependency for most folks. 👍

[giladgray]

it should also be said that the cost of a dependency is often not related to the amount of code it contains. yes we talk a lot about gzipped bytes these days, but it's quite possible that this new 1.0 API actually introduces significant performance regressions.

blueprint is also considering dropping react-popper and just using Popper.js directly. we shall see.

[FezVrasta]

I'm honestly just trying to give a better product to you guys, "it's not possible" sounds a bit harsh, considering I do it in my free time.

If anyone of you wants to help that would be great.

[oliviertassinari]

@giladgray I have been benchmarking blueprint, awesome work with the lib!
@FezVrasta The only real mistake is the one from which we learn nothing. Maybe the performance issue can simplify be fixed :).

[jquense]

@FezVrasta I'm trying! did you see #197 ? You're doing a great job. I'm not sure why everyone is showing up here all entitled, it's especially disheartening from a bunch of OSS maintainers, to show up in an issue tracker complaining and "threatening" to not use it instead of offering actionable feedback. Ya'll should know better...

[oliviertassinari]

it's especially disheartening from a bunch of OSS maintainers

This is pretty interesting. I would be eager to better understand this behavior. Is this because OSS maintainers have better empathy with other OSS maintainers? Is this because OSS maintainers have a different understanding of what OSS is? Or this something else, like OSS maintainer can be users too but with leverage and incentive. Anyway, I'm not looking for an answer. I'm just curious.

Speaking of leverage. It seems that a majority of react-popper users are library authors, not end users directly. At least it's what this data is suggesting. So my feedback would be: I wish react-popper was a lower level abstraction. Now, you can leverage my view as you wish. Eventually, the more diversity of those views, the more rational actors we have and the more of a balanced, fair community we will have :).

[FezVrasta]

About point one, I think it's that OSS maintainers know how it feels when a user comes and leaves such kind of comments, so they are the one that should understand it better and avoid to do it.

Regarding the point 2, both Popper.js and react-popper are exclusively designed to be used by library authors, the scope of Popper.js is too narrow to be useful alone, yet it's one of the most difficult things every front-end developer has to face in his career, that's why I created it rather than using any of the existing "tooltip libraries".

I think the API v1 gives much more flexibility to the library authors/consumers, while adopting a well-known pattern such as render-props are. If there are performance problems, it's not API fault, but something on the implementation.

[giladgray]

hey folks, sorry for the salty comment 😞 thanks for calling me out and sparking an interesting discussion. no excuses from me.

i really do appreciate the work in both Popper.js and react-popper and i was overjoyed to see the new 1.0 APIs here (and actively pushed for its release) and then disheartened to hear about performance issues. Popper is, in my opinion, one of the greatest OSS libraries out there: solving a hard problem incredibly well across the board. i unfortunately have few spare cycles to donate to other projects so it's easy to slip into reactionary patterns on issues outside my control.

fully agree that any issue here lies in the implementation, not the API (though i'm still struggling with how to expose Popper#scheduleUpdate() to consumers). @jquense's work in #197 looks very promising! definitely steps in the right direction.

how can i help?

[FezVrasta]

@giladgray you can expose the scheduleUpdate trough the Popper component:

<Popper>
  {({ scheduleUpdate }) => /* do something with it here */}
</Popper>

Docs: https://github.com/FezVrasta/react-popper#children

Does it answer your question?

[Justkant]

In relation to the performance problem on material-ui, I took a look at the reverted code and noticed that you were not using an instance method. (https://reactjs.org/docs/render-props.html#be-careful-when-using-render-props-with-reactpurecomponent)
Maybe this could be the root for the high number of re-renders.

[diondiondion]

you can expose the scheduleUpdate trough the Popper component:

<Popper>
  {({ scheduleUpdate }) => /* do something with it here */}
</Popper>

@FezVrasta I think what's not clear is what to do when you need to use scheduleUpdate in a parent's lifecycle method like componentDidUpdate() (or the now deprecated componentWillReceiveProps()), where you'd actually be able to conditionally trigger it depending on a change in props or state (for example when the content of a tooltip changes).
Having access to it within the render method doesn't seem very useful to me, to be honest, but I might be missing a use case.

If the ref of InnerPopper was exposed by Popper, it would be easy to trigger it using that, I guess.

[FezVrasta]

@diondiondion I guess you could just assign it to your upper component doing so:

const Upper = () => {
  let scheduleUpdateFn;
  return (
    <Fragment>
      <button onClick={() => scheduleUpdateFn && scheduleUpdateFn()}>Update</button>
      <Popper>
       {({ scheduleUpdate }) => 
         scheduleUpdateFn = scheduleUpdate;
         /* render your popper */
       }
      </Popper>
    </Fragment>
  );
}

Or, for a class component, use this.scheduleUpdateFn maybe.

[giladgray]

@diondiondion hit the nail on the head--that was precisely my confusion.

and we ended up doing what @FezVrasta suggests above but only out of necessity (palantir/blueprint#2718). setting a scope variable like that is a last resort pattern, not very React-y. a ref to the Popper would be better.

[FezVrasta]

@giladgray my example actually comes directly from the React documentation:

https://reactjs.org/docs/refs-and-the-dom.html#refs-and-functional-components

Anyway, we can add a way to get the ref from the Popper element, it shouldn't be a problem nor break any of the existing APIs.

[FezVrasta]

I just released v1.0.1, it should improve the performance of the library. May you guys give it a chance and report back please?