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

Prevent memory bloat in associations when searchable is set to false #2406

Open
adrienpoly opened this issue Jan 24, 2024 · 7 comments · May be fixed by #3418 or avo-hq/docs.avohq.io#311
Open

Prevent memory bloat in associations when searchable is set to false #2406

adrienpoly opened this issue Jan 24, 2024 · 7 comments · May be fixed by #3418 or avo-hq/docs.avohq.io#311
Labels
Bug Something isn't working Enhancement Not necessarily a feature, but something has improved Performance

Comments

@adrienpoly
Copy link
Contributor

Feature

By default an association field has a searchable: false attribute

When attaching a record. A modal loads with a select field and the associated records. This works perfectly when the list of associated records is small. But when the collection is large it is either very slow or worse can create a significant memory bloat and crash the app.

This is the typical example where locally it will run fine as you might not have lots of users in the development db but will crash in prod when displaying the association modal.

field :users, as: :has_many 

Current workarounds

After creating 2 crashes in prod, I currently ensure that all association fields have searchable True with a custom Cop.

I like the simplicity of a simple select when the collection has a reasonable size but when this size grows it should not create a memory bloat

Several solution were discussed in: https://discord.com/channels/740892036978442260/1199641108880298127

I don't have a preferred solution and understand that this could be confusing. But something should be done to avoid bloat in prod as this is a critical issue.

#2085 will help as we will be able to set a default value for searchabe. But it will still require for the user to set it and in the mean time could experience a bloat

Screenshots or screen recordings

Additional context

@adrianthedev
Copy link
Collaborator

Context

We want to give the best experience for both free and paying users, so we need this solution to fit both parties.
We also want to have the least surprise for the developer and the user. The solution should be intuitive and provide at least to the developer how and why it behaves the way it does.

The problem

If you don't use the searchable: true option on a belongs_to field, it will render a select dropdown with all the records it finds for that query. The number of records may very well be in the tens of thousands, or even millions, raising issues with loading all those records and sometimes creashing the browser.
The resolve for this is to mark the field as searchable.

The other issue is that developers might overlook or not have the proper visibility into how many records they have in different environments (eg: production).
This leads to crashing edit pages.

This happens to the attach modal of has_many as well.

Another thing to consider is that the Community tier does not have the searchable option. It's a pro feature.

Possible solutions

We'd like to offer a semi-automatic way of preventing that crash.

Options:

  1. Automatically limit the number of results to 1000 (or another arbitrary value) so it will never crash the app in prod.
  • the developer, unless they read the docs (which almost never happens), will not know that the limit is automatically applied to them
  • the developer might never hit this limit in development and will get unwanted results on production
  1. Count the number of items, limit and show a notice

After the count we can figure out if we want to apply the limit or not.

  • this will add another query to the page load (count)
  • this again might show unwanted results to the user as they don't know that this is happening behind the scenes
  1. Count the number of items and turn the field into searchable automatically (Pro users)
  • this will add another query to the page load (count)
  • this might be an unwanted behavior. the developer might not want to have that field searchable. but then again, would he like to have the app crashed?
  1. Add a global "safe" config that would turn on option 1.
  • this adds overhead of yet another config key
  • this might also bring unwanted results when it applies the limit

Things to consider

What is the arbitrary number we'd like to apply the limiting? 1000, 5000, 10_000? We should run some tests.

Please send your feedback

@adrianthedev adrianthedev moved this to To Do in Issues Jan 26, 2024
@adrianthedev adrianthedev added Bug Something isn't working Enhancement Not necessarily a feature, but something has improved Performance labels Jan 26, 2024
@adrienpoly
Copy link
Contributor Author

adrienpoly commented Feb 1, 2024

I would like to throw an idea. looking at the upcoming combobox from Jose Farias. It looks like we could have the best of both world.

https://x.com/fariastweets/status/1752494684474380717?s=20

Both free and pro user have the same dropdown :

  • free user have a non searchable dropdown with lazy loaded content (the same dropdown but the search is disabled)
  • pro users have lazy loaded content and search

To be honest I think it would be an improvement for pro users too. The current UX of the searchable field is OK but once I search for a term I am limited to 10 results and often I would like to scroll down to see more but that is not possible.

edit : the gem is not officially open but is it possible to play with it here

@Paul-Bob Paul-Bob self-assigned this Feb 5, 2024
@Paul-Bob Paul-Bob moved this from To Do to Next up in Issues Feb 5, 2024
@Paul-Bob
Copy link
Contributor

Let's check the combobox possibility and also make the 10 results after search configurable.

@adrianthedev
Copy link
Collaborator

We might be able to use something like this
https://stackoverflow.com/a/52604805/1067281

@Paul-Bob
Copy link
Contributor

Paul-Bob commented Apr 8, 2024

We might be able to use something like this https://stackoverflow.com/a/52604805/1067281

The main issue persists with that suggestions: loading all options on page load

@Paul-Bob Paul-Bob moved this from In Progress to Next up in Issues Apr 11, 2024
@adrianthedev adrianthedev moved this from Next up to To Do in Issues Jun 21, 2024
@stevegeek
Copy link
Contributor

Just throwing in my thought here on Option 1 above (regardless of the idea of adding the lazily loaded select, which sounds great):

I would personally lean on the defensive side here and always apply a limit to queries destined for rendering a table view, or UI element, eg 1000 records.

I doubt any Rails app will ever gracefully handle rendering very large numbers of records, but also the user experience of say a select box with thousands of options is so limited anyway that very few would normally intentionally create such a thing.

Anyway the limit could be configurable on a resource level or globally so could be changed if needed.

Even if the limit was unexpected in a sense, its even more unexpected to try to render a page with millions of UI elements, so my guess is that this is a much bigger problem than the few devs who actually wants a select box with 10,000 options in it.

@adrianthedev
Copy link
Collaborator

Ok.
We can add this protection to keep it from blowing up.

Approach

  • Let's automatically add a limit to the query
  • the limit should be configurable in the initializer (globally)
  • let's append another disabled option at the end if to tell the user that there are more elements available but they won't be displayed it the limit si surpassed. "There are more records available."

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working Enhancement Not necessarily a feature, but something has improved Performance
Projects
Status: To Do
4 participants