RFC: Turn eager_load on for development

Development RFCs
#1

Hello,

when you boot Foreman in production, eager_load configuration value is turned on therefore all application classes are loaded. However this is not enabled therefore many classes loads as they are needed, most of them during the initial request.

Here is a problem - we have some features which lists all classes (e.g. descendants of a class) during boot. This creates situations when Foreman does not behave the same as in production and it can lead to confusion for users who don’t know about this problem. For example, when working on Subscription API (Foreman events and webhooks), you actually need to turn on eager_load to be able to do anything reasonable.

I hereby propose to turn on eager_load for development environment.

In my testing, Foreman boots up in 10 seconds without eager_load and in 15 seconds with it. That’s 50 % slower boot. I know it’s much to ask, before you stop reading keep in mind that initial load of a page (Hosts list) is 25 seconds without and 22 seconds with eager_load on my Foreman. If you put this into the context:

  • currently: 10s + 25s = 35s end to end load time before you see a rendered page
  • the proposal: 15s + 22s = 37s end to end load time before you see a rendered page

Now that’s not that bad considered the advantages we can get. But I am not stopping here, I created a patch that postpones loading of all gettext locales to the point when user actually needs them. This shaves off 2 seconds off the boot time in development. This puts the total boot time on my system back to 35 seconds.

Let me know what you think and please someone measure your boot times with and without the option in question to validate my numbers.

#2

I wonder if @aruzicka’s work around Zeitwerk would actually fix the descendants problem.

#3

It could, depending on who we approach it, but the zeitwerk stuff is far from done so I wouldn’t count on it in the near future.

#4

I’m for turning on eager load and in general anything that makes the development env closer to production. Who knows, maybe if devs feel the time it takes to load in production they’ll also improve it :wink:

1 Like
#5

Without the patch
9s the start + 4s the first page load (dashboard, login was much faster)
with the patch
9s the start + 3s the first page

In fact in my case I didn’t notice any difference, the second load times may be slightly faster due to various caching mechanism not under our control. Given I wouldn’t notice anything, I’d say that’s fine and brings us closed to the production like setup. :+1: from me.

1 Like
#6

Allright, here is the PR, let’s keep the conversation going in the meantime.

Thanks all.

#7

We discussed this on a todays meeting and there was a consensus to set eager loading also for tests so we have all the three environments with the same behavior. So I rebased the patch.

This could slow down testing of a single Ruby file or test a bit, let me know what you think. According to my measurements, loading all Foreman core + bunch of plugins is 2 seconds.

#8

I think it’s reasonable, it hurts, but we will be closer to the prod env.
Full disclosure, I’m for not eager loading in test env, but I get the arguments for it, so I’ll merge with eager_load = true in all envs :+1:

1 Like
#9

So there’s a problem with enabling eager_load in tests environment. Rails won’t boot, @ezr-ondrej explained to me it’s because how we implemented settings. Looks we’d need to reimplement settings. So it’s either merge without test env or drop the PR and this proposal.

#10

Permissions, but yeah as of now, we can’t easily turn this on in test.

#11

Opinions @aruzicka @Marek_Hulan @tbrisker ?

#12

I would say let’s turn it on for dev at least and try to figure out if we can how to also fix tests. The closer to production that we develop and test is better imho, even at the cost of a couple extra seconds when starting the server.

2 Likes
#13

Can we file an issue describing it and link that somewhere? Perhaps even in environment/test.rb right above where we disable it since this is the kind of information that gets lost over time quite easily.

2 Likes
#14

Sure: Bug #31977: Turn on eager_load for test - Foreman

I will rebase the PR then. It’s a start, as much as I would love to see it also for test I have no idea what’s wrong.

#15

I’m fine with changing dev env only.

1 Like