daddyz / phonelib

Ruby gem for phone validation and formatting using google libphonenumber library data
MIT License
1.08k stars 132 forks source link

is there a way to opt out of eager loading? #292

Closed camallen closed 10 months ago

camallen commented 11 months ago

Related to https://github.com/daddyz/phonelib/pull/288

I'm seeing a large memory jump in our rails app since the introduction of eager loading as it loads the data files into memory. Is there a way to opt out of this?

senny commented 11 months ago

After upgrading the dependencies of our app we found that our baseline memory footprint has increased by a large mount. Bisecting the bundle update, it turns out that the root cause was upgrading from phonelib version 0.8.5 to 0.8.6:

before (0.8.5):

irb(main):001:0> `ps -o rss= -p #{Process.pid}`.to_i
=> 255520

after (0.8.6):

irb(main):001:0> `ps -o rss= -p #{Process.pid}`.to_i
=> 372000
fsmanuel commented 11 months ago

I also observed a memory increase of an app from ~270 MB to ~370 MB and growing. @tgwizard is there a way to opt out of the eager loading?

tgwizard commented 11 months ago

I'm sorry, I'll be able to have a look in the new year, but perhaps someone more familiar with the library can look into it before then?

My assumption in that PR was that using the library would load those files anyway, so you'd have that memory footprint after first use. If that's not true - if not all those files are commonly used - then they arguably shouldn't be eager-loaded automatically. @daddyz ?

@fsmanuel not sure I see how the memory footprint can keep growing as part of this change. Eager loading happens only once per process.

fsmanuel commented 11 months ago

@tgwizard you are absolutely right. A little more context: The rails app runs in a lambda container and the used memory jumped from ~270 MB to ~370 MB. Somehow it was also growing over time. Both started with the same deployment but don't have to be related.

The lambda only handles background jobs and never needed phonelib so I never observed the memory jump in the past.

daddyz commented 10 months ago

@camallen @fsmanuel @tgwizard @senny I've added a Phonelib.skip_eager_loading! in v0.8.7 method, add it to initializer in order to skip eager loading. I tested on dummy app, there is a diff of ~100M which is also something that you observed.

camallen commented 10 months ago

@daddyz thanks for this 👏🏼 🎉

okirmis commented 10 months ago

Maybe this should be designed as an opt-out (like Phonelib.enable_eager_loading!) as increasing the rails application memory by 50% was in our case a breaking change... breaking as in "our app broke due to memory limits". The Phonelib.skip_eager_loading! method could still be there for compatibility, so basically the previous default behavior could be restored while giving people the possibility to have eager loading enabled (which in some case may definitely be a good idea).

While in our case the staging deployment caught this, changing this behavior in patch version seems not like a good idea if I'm honest (took us a while to find phonelib to be the culprit here, wasn't my first guess ;) ). Especially when you have multiple puma workers, the memory adds up pretty quickly.

I would create a PR, if you like.

spoik commented 5 months ago

We ran into memory issues as well after introducing this gem. Skipping eager loading via Phonelib.skip_eager_loading! seems to have helped. I think it would be worth mentioning the memory impact of the gem and the option to skip eager loading via Phonelib.skip_eager_loading! in the README. Perhaps it would be best to change eager loading to an opt-in instead of being enabled by default. It would have been helpful for us but I can't be certain it'd be helpful for a majority of the gems users. I believe this is what @okirmis was getting at in the previous comment.