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

[Laravel] Service Provider performance #1776

Closed
barryvdh opened this issue Jul 6, 2019 · 4 comments
Closed

[Laravel] Service Provider performance #1776

barryvdh opened this issue Jul 6, 2019 · 4 comments
Milestone

Comments

@barryvdh
Copy link
Contributor

barryvdh commented Jul 6, 2019

Hello,

So I'm probably nitpicking here about 0.5ms, but anyways, Re: #1363

In light of checking the Laravel boot performance, I was checking which service providers are making the biggest impact on the boot time for a 'hello world' request. The Carbon ServiceProvider stood out because it took more time then other providers. (And because it is required by Laravel itself, but auto-registered, it's easy to miss because it's not there with the rest).
I'm not sure what setLocale actually does, but it seems to create a Translator instance and load the message from disk. In regards with performance on an optimized config (see https://twitter.com/barryvdh/status/1147547152366874628) I get +/- 3ms to reach the Controller. If I put Carbon in the dont-discover list, it drops about 0.5ms. Which obviously isn't a lot an absolute terms, but it's still about 16% of the entire boot of Laravel.

Removing the ServiceProvider now would obviously break lot's of projects, but does the setLocale need to be this heavy? Am I doing something wrong (seeing the result on opcached production, but also on Valet+)?

Possible solutions:

  • Defer the Translator/Message until it's actually used (it's just for formatLocalized / diffInHumans, right?) Perhaps set the Locale as a variable on the factory and use that when creating the translator when it's required.
  • Cache the messages using Laravel (eg. in bootstrap/cache/carbon.php or storage/framework/carbon)

Issue Tempate:
I encountered an issue with the following code:

    public function updateLocale()
    {
        $translator = $this->app['translator'];

        if ($translator instanceof Translator || $translator instanceof IlluminateTranslator) {
            Carbon::setLocale($translator->getLocale());
        }
    }

Carbon version: 2.20.0
PHP version: 7.2.8 / 7.3.6

I expected to get: No performance penalty for booting Laravel
But I actually get: 0.5ms 'delay' when using the ServiceProvider

@kylekatarnls
Copy link
Collaborator

kylekatarnls commented Jul 6, 2019

it's just for formatLocalized / diffInHumans, right?

Nope, it's a lot more methods (all week methods for example will use first_day_of_week and day_of_first_week_of_year of the locale), getters for month and week days names (short and long), isoFormat, parseFromLocale, translateTimeString, it's used by CarbonInterval and CarbonPeriod. So intercepting all this function to lazy load the messages is not really an option.

Cache the messages using Laravel

Not an option neither as we don't know in advance the locale (it can be any language with any region among hundreds). Cache is app-specific, the locale is in most cases user-specific (get from BD, session, cookie, etc.)

If your app is 100% in English or does not need any translation for date, adding Carbon ServiceProvider in dont-discover is a good option. The provider is here so Carbon will use the locale of your app by default, so at the boot, it takes your app.locale setting and when you run App::setLocale('fr') to change the i18n locale of your Laravel app, the locale used by Carbon changes too.

I expected to get: No performance penalty for booting Laravel

You can only get this using dont-discover as no code can be executed instantly. Even loading the provider takes time. We can check if we can make it faster without losing the locale synchronization but it will never be 0µs.

@barryvdh
Copy link
Contributor Author

barryvdh commented Jul 6, 2019

Sorry, I shouldn't have said 'No penalty', I indeed mean very little.

I do now realize that part of the time might be from actually loading Carbon itself, which is triggered by the setLocale, instead of later when Carbon is first used, so the penalty 'in real life' would probably be smaller.

@kylekatarnls
Copy link
Collaborator

However there are still good improvements in your PR, maybe I will keep all but the setDefaultLocale method. Then working on translations performances globally (not in the particular case of the Laravel boot). It's in the road-map:

Improve performances (bottlenecks such as loading translations files).

@kylekatarnls kylekatarnls added this to the 2.21 milestone Jul 6, 2019
@barryvdh
Copy link
Contributor Author

barryvdh commented Jul 7, 2019

Hmm, it's hard to pinpoint exactly, but I think it's probaby not the Locale call itself, so closing this for now. Thanks for the feedback!

@barryvdh barryvdh closed this as completed Jul 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants