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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

resolve_entries is slow when using vite_javascript_tag for very large and complex tsx react/relay files #416

Open
2 tasks done
SemmyTan opened this issue Nov 6, 2023 · 2 comments
Labels
good first issue Good for newcomers

Comments

@SemmyTan
Copy link

SemmyTan commented Nov 6, 2023

  • I have tried upgrading by running bundle update vite_ruby.
  • I have read the troubleshooting section before opening an issue.

Description 馃摉

Provide a clear and concise description of what the bug is.

The method resolve_entries in lib/vite_ruby/manifest.rb (https://github.com/ElMassimo/vite_ruby/blob/main/vite_ruby/lib/vite_ruby/manifest.rb#L27) is very slow when it is called from the vite_rails method vite_javascript_tag for a very complex .tsx file that uses react and relay modern. The .tsx file in my case imports many levels of nested children components - the resulting output from the single vite_javascript_tag method call is over 300 <link rel="modulepreload" ... tags and a few dozen stylesheet tags. The actual web app runs smoothly so there aren't any performance issues that I can see with it.

The cause of the slowness in my case is the .uniq at the end of this line in the resolve_entries method:

imports = dev_server_running? ? [] : entries.flat_map { |entry| entry['imports'] }.compact.uniq

I inspected the object that .uniq was being called for in my case and it was an array of over 300 hashes. Some of these hashes were gigantic - the 5 largest ones were between 100 and 300 million characters long when I ran .inspect.length on the hashes. .uniq seems to be very slow when comparing multiple gigantic hashes - the run time in my case varied between 6 to 12 seconds depending on the specs of the hardware I was testing with.

The fix for my case was simple - I just removed the .uniq at the end of that line. The 2 spots the imports variable is used further below in that method both have their own .uniq call on the resulting value, so the first .uniq when defining imports = ... does not seem necessary. I added this monkey patch to my code base, but thought it might be a useful change to add to the vite gem if there aren't any problems with this change. I can make a pull request for this change if it seems fine.

Reproduction 馃悶

Please provide a link to a repo that can reproduce the problem you ran into.

Unfortunately, I am not able to share the code base where this is happening and creating a new repository large and complex enough to replicate the issue does not seem to be a trivial task.

Logs 馃摐

If not providing a reproduction:

Output I added logging to check the length of the hashes. In the private code base I am working with, this resulted in an array of over 300 numbers. The 5 largest ones were between 100 and 300 million characters long each. Summing the lengths resulted in 1.3 billion. I know `.inspect` includes all the syntax characters such as `{` which causes this to be longer than the real data, but numbers this large still show that the hashes are ridiculously large.
...
imports = dev_server_running? ? [] : entries.flat_map { |entry| entry['imports'] }.compact.uniq
Rails.logger.info imports.map{|hash| hash.inspect.to_s.length}.sort
Rails.logger.info imports.map{|hash| hash.inspect.to_s.length}.sum # outputs 1314704174
...
@SemmyTan SemmyTan added the bug: pending triage Something doesn't seem to be working, but hasn't been verified label Nov 6, 2023
@ElMassimo
Copy link
Owner

Hi Semmy, thanks for reporting.

Given that we call uniq on the JS imports and the CSS stylesheets later on, it sounds reasonable to remove the uniq call
on line 31
.

@ashaninBenjamin
Copy link

ashaninBenjamin commented Feb 8, 2024

We've just faced the same issue. It takes about 9s to take unique elements.
However, there are only 127 entries on the 1st level.
The manifest file has 12k lines, it's not enormous.

We added a monkey patch to avoid using the Array#uniq method.

@ElMassimo ElMassimo added good first issue Good for newcomers and removed bug: pending triage Something doesn't seem to be working, but hasn't been verified labels Feb 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

3 participants