[useDirection] Eager hook is called on every state change.

[steveruizok]

Bug report

Current Behavior

Several components (such as the DropdownMenu component) use the useDirection hook.

If this hook does not receive a dir prop, the hook reads the element's computed styles by pulling the direction property from the element's computed style.

This can be expensive. As none of the elements provide a default for this prop, if used on an element that might changes very often for whatever reason, this will cause the hook to run on every frame. This was happening to me: see a before and after before I went source-code spelunking.

Before (with <Dropdown.Root>): Video Link

After (with <Dropdown.Root dir="ltr">): Video Link

Expected behavior

If no property is provided, the hook should stick with its first result rather than repeating the expensive job of getting the element's computed styles.

Reproducible example

Extremely contrived
https://codesandbox.io/s/nervous-jackson-plg87?file=/src/App.tsx

Suggested solution

At the risk of being a touch imperialist, I'd expect ltr to be the default for the dir prop.

If you do want to read the element itself to get the direction, then I'd expect that the hook did not refresh when the element changes. Specifically, I'd suggest that element be dropped from this dependency array.

Otherwise, if you want to foreground the ltr or rtl choice, make it a required prop.

If none of the above are possible, I'd expect to see this flagged as a potential performance issue when leaving it undefined for components that may change often.

Your environment

Software	Name(s)	Version
Radix Package(s)	@radix-ui/react-dropdown	0.0.22
React	n/a	Latest / see sandbox
Browser		`
Assistive tech		`
Node	n/a	`
npm/yarn		`
Operating System		`

[benoitgrelard]

Hey @steveruizok, thanks for that.

Before we can look much further, can you help me understand why the element itself might change often? Which use-case/primitive did you find that to be the case?

[steveruizok]

I believe it was the DropdownMenu that caused it, though I'm not sure which. I have a few menus that are shown conditionally in the app, depending on how many items are selected, but none of those options would have been changing during a drag session. If I had to guess now, I'd say it was the style panel dropdown at the top right, which currently updates whenever selected elements change.

I'll look back at the code (perhaps it's indicative of a different issue). It seemed like anything that caused the element to render, like an unmemoized callback or object prop, would also cause the hook to run.

[benoitgrelard]

I'm asking because although the hook will run on every render, the expensive part getComputedStyles will only run again if the element changes. This is ok because the style declaration you get from getComputedStyles is a live declaration, meaning that it is always up to date without having to incur another call to getComputedStyles.

So the hook code basically can be summed up as:

• run getComputedStyles if the element or the dir prop itself changes (most likely would be only once alltogether)
• if the dir prop isn't defined directly (inherited) we continuously read it from the cached live computedStyles in a raf loop

Reading it shouldn't be expensive as far as I know, so that's why I'm wondering if it's something else you're seeing here.

[jjenzz]

As Benoit has already explained, getComputedStyle will only be called once because the element should not change on re-render. I've done some tests locally with your reproduction code and also confirmed this there.

I'm getting the following Call Tree results:

Screenshot	Results
	The getStorybookSetCountLoop (the loop from your codesandbox that is updating the count and triggering the re-renders) is ~0.7ms of the total time while our getDirection loop is only ~0.4ms.

If I remove all of our direction logic so that the effect is simply calling an empty requestAnimationFrame loop, there isn't a noticeable improvement in perf so it seems the issue you're experiencing could be somewhere else.

Looking at the code you have written to reproduce the issue, are you suggesting that the drag logic in your app is updating some React state over and over again with the desired position of the shapes you are rendering on screen? It's usually not recommended to trigger lots of re-renders for things like that so if you are, improving things there might help with perf issues you're experiencing.

[jjenzz]

Closing this for now because it's unlikely that the rAF loop is the source of the performance issues. Happy to reopen this though if some use-cases can be provided that show otherwise.

[dwilt]

@steveruizok thank you so much for being a touch imperialist - you saved me and my dev team lots of digging around into a very large performance problem in our app. We have dozens of instances of the Radix Dropdown menu in our app:

I took the below performance recording while doing nothing in our web app. I just hit record. I didn't even move the mouse over the application - it never left the dev tools. You can see hundreds of rAF calls clogging up the JS thread running on every frame.

@jjenzz I don't think your performance measuring was really fair because you only have 1 instance of the dropdown menu on the page and you weren't running the CPU at 6x slowdown. It's a known good practice to be using 6x slowdown on the CPU when doing performance testing because you (as a developer) likely have a much faster computer than a lot of regular users.

We applied the dir="ltr" to the Root component and it did fix the performance issue. Agree with @steveruizok that this useDirection hook should have a default value for dir. As a library who probably strives to be used by large companies in production-grade applications, I'm surprised that this issue was closed saying that this performance issue wasn't important.

[benoitgrelard]

Hi @dwilt,

Thanks for giving details of your experience with this.

…a very large performance problem in our app.

Can you give more details about what performance issue you saw? Or is just that you opened the profiler and saw the trace there which worried you?

From what I can see in the gif where you're using your app, there doesn't seem to be any issues (that I can see). Similarly to my original response to Steve, I'm interested in understanding how you got to the conclusion, what you observed, etc.

I took the below performance recording while doing nothing in our web app. I just hit record. I didn't even move the mouse over the application - it never left the dev tools. You can see hundreds of rAF calls clogging up the JS thread running on every frame.

I think overall I still stand by my previous comment (#749 (comment)).

There's a raf loop going on, so you will see things happening in the trace. If you have a raf loop running, even with no code inside, you will still see something in the trace. If you look carefully at your trace, you can see that each event is in fact in the order of μs (or at the worst below 1ms, and that's with 6x CPU slowdown).

@jjenzz I don't think your performance measuring was really fair because you only have 1 instance of the dropdown menu on the page and you weren't running the CPU at 6x slowdown. It's a known good practice to be using 6x slowdown on the CPU when doing performance testing because you (as a developer) likely have a much faster computer than a lot of regular users.

I also ran my own tests (we have stories with hundreds of instances of menus) with 6x CPU slowdown and I see the same results. So I would be surprised if it's creating meaningful performance issues.

I'm surprised that this issue was closed saying that this performance issue wasn't important.

Unless there's a real concern about performance, we are unlikely to make a change. Especially given there's a very simple escape hatch to opt out of the whole "watch for inherited direction" mechanics (the raf loop).

Perhaps, you have other ideas as to how we could reliably detect a change in inherited dir without using a raf loop?

I can think of ways we could "mitigate" things by running it only where explicitly needed (for example we may get away with only running it when the menu is opened, as we probably don't explicitly need the dir for the trigger). We could also explore a global provider approach, etc. But again, we generally only optimise things when we actually observe that there's an issue.

I hope this helps, and if you do have more details about it being an actual perf issue, we would love to understand and help resolve it.

[jjenzz]

@jjenzz I don't think your performance measuring was really fair because you only have 1 instance of the dropdown menu on the page and you weren't running the CPU at 6x slowdown. It's a known good practice to be using 6x slowdown on the CPU when doing performance testing because you (as a developer) likely have a much faster computer than a lot of regular users.

We also gave this a run through web-vitals (built by folk who know a lot more about perf than me) in a CRA setup with 500 DropdownMenus and this is the result:

• Without the loop
  • Time to first byte = ~6ms
  • First contentful paint = ~3500ms
  • Largest contentful paint (when opening a dropdown) = ~3300ms
  • First input delay (when opening a dropdown) = ~3ms
• With the loop:
  • Time to first byte = ~6ms
  • First contentful paint = ~3500ms
  • Largest contentful paint (when opening a dropdown) = ~3500ms
  • First input delay (when opening a dropdown) = ~10ms

We're talking 100s of menus here at 6x slowdown and there is negligible difference with or without the loop. Clicking around also feels responsive. So, we're keen to understand how this loop is the source of your performance issues.

As @benoitgrelard has said though, there may be ways we can avoid these discussions in future so we'll re-open the issue for more input. I can get behind an argument that this makes debugging performance issues more difficult because of all the noise 😅

[dwilt]

Can you give more details about what performance issue you saw? Or is just that you opened the profiler and saw the trace there which worried you?

Check out this video showing the performance problem we were seeing before applying the fix. Hopefully this helps. If you need anything else from me, let me know.

Perhaps, you have other ideas as to how we could reliably detect a change in inherited dir without using a raf loop?

Why not just provide a default value (of ltr) so it's not undefined?

[benoitgrelard]

Check out this video showing the performance problem we were seeing before applying the fix. Hopefully this helps. If you need anything else from me, let me know.

Thanks this is great!! There’s definitely something going on.

Why not just provide a default value (of ltr) so it's not undefined?

Because if we provided a default then it would kill inheritance.

Thanks again for this, we’ll investigate and try and find a different approach.

[dwilt]

@benoitgrelard Take a look at this video too. I think this is a clue as to what might be going on.

[benoitgrelard]

Wow that’s really odd. I’d love to see the profiling of those actions to be honest, that’s where I’d look first, ie. What is happening when it’s trying to open that context Menu or that dialog?

[dwilt]

@benoitgrelard probably easier to answer that if we got on a screenshare. feel free to send me an email and we'll figure it out