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

[5.9] Per environment config caching #28998

Closed

Conversation

Projects
None yet
4 participants
@timacdonald
Copy link
Contributor

commented Jun 30, 2019

Replaced by: laravel/laravel#5050

This PR is part one of two PRs (part two). Together these PRs can reduce the boot time of the Kernel during a test run by ~50% without any additional work by the developer. This will only really have an impact on a larger test suite, but personally I think it is a worthwhile thing to consider.

I recently did some benchmarking (which I wrote about over here) of Laravel's TestCase class. Naturally, as we would expect, extending Laravel's base class instead of PHPUnit's base class comes with some overhead as the Kernel is being booted before every test.

I found that I was able to reduce this overhead by 50% if I first cached the configuration, just like you would in production. The test suite ended up with the following durations:

  • Without caching: 9.15s
  • With caching: 4.2s

Doing this is good for a couple of reasons:

  1. It's faster.
  2. It is closer reflects what you use in production.

There are however a few issues.

  1. Whenever you cache the config it always ends up in bootstrap/cache/config.php so when you cache your testing environment config it overrides any local config caching. This means that your testing config is now used when using your app locally (unless you clear the config cache of course).
  2. The process of caching / clearing the config before and after a test run is a manual one.

I am proposing we introduce 2 new features.

Per environment config caching

When you cache the config, it will now use the following cache files...

php artisan config:cache
# bootstrap/cache/config.php

php artisan config:cache --env=testing
# bootstrap/cache/config.testing.php

php artisan config:cache --env=whatever
# bootstrap/cache/config.whatever.php

A PHPUnit Listener

The PHPUnit listener will cache the config for the test environment before a test is run. Because it is in "User land" it is 100% opt-in. If you don't want it, don't use it.

One handy thing about running the cache command in a PHPUnit listener (as opposed to say in a composer script) is that it will already have the environment variables from the phpunit.xml file loaded, so we don't need to read those ourselves.

The PR for the listener is over here: laravel/laravel#5049

Notes

  • There is an environment variable to override the config cache path. I'm not entirely sure the best way to use that existing override to allow this to all happen behind the scenes.

  • I'm checking if we are running in the console in these adjustments so that we don't affect production speeds while trying to improve test run speeds.

  • This PR might need adjustments depending on the outcome of #28982

  • Am I crazy or are there no existing test for the config commands? Is this on purpose or would you like me to write some tests for the new functionality?

  • It should be possible to achieve all of this from a 3rd party package, but I think it is something that would actually be handy in core. If it isn't suitable, I'll attempt to package it up.

  • It would be great if someone familiar with complex environment setups in CI / production could give any feedback on this. My env setup is always pretty basic TBH.

Further improvements

If this kind of thing is wanted in core, there is also potential to cache routes, events, etc. But I wanted to see if it was wanted before putting in the work to do all of them.


Edit:

An alternative approach

In my first run at this I was having issues using the APP_CONFIG_CACHE but I just had another run at it and this improvement looks like it is possible without any changes to the existing framework.

This new PR to laravel/laravel does the same thing as this 2 part PR - all in one, i.e. you don't need any changes to laravel/framework.

timacdonald added some commits Jun 30, 2019

timacdonald added some commits Jun 30, 2019

@matthewnessworthy

This comment has been minimized.

Copy link

commented Jun 30, 2019

@timacdonald would you not be able to set the env var APP_CONFIG_CACHE with a testing-specific path when running your tests instead?

e.g.,

APP_CONFIG_CACHE=/test/cache/path/for/config.php php artisan config:cache
phpunit
APP_CONFIG_CACHE=/test/cache/path/for/config.php php artisan config:clear
@timacdonald

This comment has been minimized.

Copy link
Contributor Author

commented Jun 30, 2019

@matthewnessworthy I did consider that variable (and mentioned it in my first comment) but I ran into some issues. That being said, I didn't actually document what they were, and I've also change my approach and have a better understanding of this part of the system, so it might be worthwhile giving it another spin. Will report back shortly.

@@ -23,11 +23,11 @@
* @method static void booting(callable $callback)
* @method static void booted(callable $callback)
* @method static void bootstrapWith(array $bootstrappers)
* @method static bool configurationIsCached()
* @method static bool configurationIsCached($env = null)

This comment has been minimized.

Copy link
@GrahamCampbell

GrahamCampbell Jun 30, 2019

Member

Should be string $env = null.

* @method static string detectEnvironment(callable $callback)
* @method static string environmentFile()
* @method static string environmentFilePath()
* @method static string getCachedConfigPath()
* @method static string getCachedConfigPath($env = null)

This comment has been minimized.

Copy link
@GrahamCampbell

GrahamCampbell Jun 30, 2019

Member

Same here.

@timacdonald

This comment has been minimized.

Copy link
Contributor Author

commented Jun 30, 2019

@matthewnessworthy I'm not sure what issues I was running into. I just made a complete working version in like 2 minutes with that env variable 😆

@mfn

This comment has been minimized.

Copy link
Contributor

commented Jun 30, 2019

2. The process of caching / clearing the config before and after a test run is a manual one

Is this still the case?

If the environment variable to a different cache file works, it's only relevant to clear the config cache once before the test suite which should be rather easy to detect.

@mfn

This comment has been minimized.

Copy link
Contributor

commented Jun 30, 2019

I've a test suite of ~6k tests to this test on though there are (unfortunately) a LOT of integration tests so not sure if the framework boot time is even relevant. But I try to see if I can test it until tomorrow.

@timacdonald

This comment has been minimized.

Copy link
Contributor Author

commented Jun 30, 2019

Gonna close this in favour of this alternative approach which has a much smaller footprint on the framework: laravel/laravel#5050

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.