-
Notifications
You must be signed in to change notification settings - Fork 1.3k
RAC - Table performance very slow when rendering new collections. #8094
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
Comments
Not sure why, but that |
Adjusted the example to extract the logging inside a memo'd component with just the Table component If you uncomment the Edit: I can look into setting up a more minimal reproduction in a basic vite app tomorrow if that's easier to work with. |
After investigating this a bit more, it seems to have something to do with using Updated example to showcase difference: https://stackblitz.com/edit/remix-run-remix-cmjxmb21?file=app%2Froutes%2F_index.tsx This only started happening from 1.8.0 RAC release so I'm unsure if I should file a bug with react-router or keep it here? |
Thanks for the additional information. I'm unsure as well, not being well versed in react-router. We did make changes to rendering in 1.8, though it should have all been performance boosting, so either we introduced a weird regression or yeah, the issue is in react-router. Can't hurt to open an issue against them, though I'd try to remove our library from the example if you did that. |
As far as I can tell, this is only happening with the table component within our 2 apps that use react router + RAC. So unsure how to make a reproduction without RAC to react-router, but I'll run some performance tests to try and trigger it elsewhere. |
@Jarrku I think you're on to something.... I'm running into something similar but with RAC and NextJS. I have a Table that updates the @snowystinger It may not feel like much in this very stripped down code sandbox but check out the performance flame chart for something as simple as this. https://codesandbox.io/p/devbox/modern-wood-ywh7l6?workspaceId=ws_VeyR4ykdaWme1iMQigmAsK The re-renders are really quite prominent after typing or removing the first letter in the search (I've been using a capital "P" to filter for words that start with "Predicted") even if the performance is not noticeable in the code sandbox but really start to fester in larger settings. I get a recursive flame chart many hundreds deep of
Here is a screenshot of me on a ![]() My gut just feels like this is an excessive amount of javascript execution for filtering a table And here is the JSON trace dump if you want to throw it in to dev tools too (I couldn't upload directly): https://drive.google.com/file/d/1Vhb7DNS_XSKzqmA1ywSivSWAV1Mm4MTa/view?usp=sharing Just for reference here is a screenshot of the equivalent action (which led me to this issue) on my dev site where just typing a letter in to the input caused an almost 700ms interaction delay because of all the js activity :' ) ![]() Let me know if I can help provide anything else! |
just realized that the codesandboxes ive been sharing on tickets were all private so just wanted to share that the one in my message above should be public now...sorry |
No worries! Just as an update, we've done some digging and it seems to be an issue with our update to support Suspense and how React is handling Transitions updates. For instance, if you wrap the state setter in the original example in a @uncvrd I'm unable to get that codesandbox to run, getting a 502 in the preview... |
@LFDanLu glad you were able to at least pin point the problem! Re Codesandbox: so yea i ran in to that too, thought it was just me when I set the sandbox to public today. Click a key to close the errored terminal and then run Hopefully that works? Let me know! |
@uncvrd Thanks, got the sandbox working! From the code, I would guess that this isn't the same issue as the transitions one I mentioned earlier. It is definitely concerning that you experienced such a delay on your dev site above, how much data was being filtered and displayed in that table there? Is it the 30ish rows that you mentioned previously? |
@LFDanLu ok sweet, but yea that's correct, it's just 34 rows It's actually kind of hard to get a performance monitor because doing this causes the recording to crash 2 times out of 3. Ran it again just by typing P and then the highlight below is from hitting backspace... ![]() I use Algolia for search results, maybe it has to do with how the items array is being passed? Basically when you type you need to call
The search field component is using the RAC Search Field, and upon
|
@uncvrd Hm, interesting. I can't reproduce that degree of slowness locally in our storybook with the same table setup both before and after the PR changes, not sure if NextJS is doing something here. You can perhaps try the nightly that will be generated for Just to make sure though, you weren't seeing this issue prior to the 1.8 release correct? |
@LFDanLu thank you for helping to look in to this by the way...let me see if I can recreate the same flame chart locally with NextJS. You might be on to something regarding NextJS having a say in this. So I'll try comparing a flame chart between a dev server and prod build to see if a prod build negates this in the performance chart. Also unfortunately can't confirm if this cropped up in 1.8. I started migrating in 1.7 from Radix but hadn't really messed with anything RAC collection related until just recently :/ |
@LFDanLu I was able to confirm that this is indeed an issue with NextJS in DEV MODE. Creating a production build and running the same exact test outputs significant performance improvements: 600ms delay -> 60ms...insane Reproduction: https://github.com/uncvrd/rac-collection-bug In Dev mode ![]() In prod ![]() I've uploaded the dev/prod Chrome traces to this Drive folder: https://drive.google.com/drive/folders/1YeJdvpTuHq-YAafXu6i2P5L1Cq3tlC6H?usp=sharing Not sure if it's something worthy of inspection on RAC side but maybe feel like this might come up with others? Actually surprised it hasn't though... EDIT Okay it's not just NextJS, I was able to recreate the same issue on Vite Reproduction: https://github.com/uncvrd/rac-collection-bug-vite Vite performance output ![]() My test case is
This outputs about 500-600ms of a delay on Vite & NextJS in dev mode Hope this helps a little bit? |
@LFDanLu Hey! apologies for the ping, since this is a closed issue, do you think I should open up a new ticket for this? Sorry...just wanted to make sure this didn't get overlooked since it's closed now. If youre busy no worries on that front either haha Thanks a lot |
Sorry about the delay, the team is a bit busy with preparations with the next release. I think a new ticket would be appropriate here, thanks! |
@LFDanLu no worries, on it! |
Uh oh!
There was an error while loading. Please reload this page.
Provide a general summary of the issue here
Latest react-aria-components seems to have introduced something that affects dynamic rendering of table elements.
It seems this only seems to happen in Dev mode. When running in production mode, I can't seem to trigger the problem.
🤔 Expected Behavior?
I expect the table to trigger some rerenders when changing items, but not with the slowness currently present.
😯 Current Behavior
Table rerenders very slowly when using items with new IDs
💁 Possible Solution
No response
🔦 Context
No response
🖥️ Steps to Reproduce
https://stackblitz.com/edit/remix-run-remix-cmjxmb21?file=app%2Froutes%2F_index.tsx
Toggling a search column triggers a new data set.
Version
1.8.0
What browsers are you seeing the problem on?
Chrome
If other, please specify.
No response
What operating system are you using?
Mac
🧢 Your Company/Team
No response
🕷 Tracking Issue
No response
The text was updated successfully, but these errors were encountered: