Performance enhancement for HTML Class. #1180
Conversation
…ls to Config::get();
Calling the "Config::get('application.encoding')" is expensive and within a large form (using the form builder) having it requested multiple times can result in a significant performance drag.
Caching this value reduced calls to Config:get within our project from 1200+ to 125. All core tests appear to pass with this change in place.
|
Nice pull, will let Taylor have a peek! Thanks for providing plenty of info :) |
|
PS This was the performance issue I alluded to in the Laravel IRC chat last week and promised to e-mail Taylor. But rather a patch than an e-mail! |
|
Good idea! How about a static method in this class that returns the charset, from the static variable if it is set, else from the config. Keeps the repetition out of there and is prettier... |
…ass (in order to remove duplication of logic). All tests continue to pass.
|
Hello, I've just committed an additional change to conform with franzliedke's suggestion. The new logic has now been localised in to a get_encoding method to avoid unnecessary duplication. Hopefully this is okay, |
| * | ||
| * @return string | ||
| */ | ||
| public static function get_encoding(){ |
There was a problem hiding this comment.
Bracket on it's own line and should be able to use protected here.
|
In response to crynobone, I've now resolved the coding style issues with my original implementation. (Bracket is now on its own line and method scope is protected). Thanks, |
|
Nice one! I'd suggest you change return static::$encoding ?: static::$encoding = Config::get('application.encoding'); |
|
Calls to Config::get() aren't all that expensive; the Config class is already caching the config. |
|
Hello, I've renamed get_encoding() method to just encoding() as per JoosK's suggestion, although I left the syntax as it was as it seems more readable that way. I'm more than happy to change it if an alternate syntax is preferred though. Thanks, P.S. @kbanman In a small application I agree that the speed hit is negligible, but when scaling those small speed hits add up in a very noticeable way. Just as an example, benchmarking the Config::get("application.encoding") against a standard array access for the same value, for one million calls, will result in the config::get method costing 4.515 seconds compared to 0.139 seconds for the direct array access. (roughly 32 times slower). Its also fair to note that the slowdown will be more significant the deeper the config item is nested, ie. getting the value for foo.bar.test will be slower than foo.bar. Although its true no reasonable application is ever likely to be making anywhere near a million calls to the config::get method, if you scale the application up to a few 100 concurrent users and consider that the method can be hit upwards of a 1000 times a page load, as far as the server is concerned it may as well be. Hence while for many applications saving some calls to the get::config method likely wont make any noticeable difference to the page speed, as soon as you start scaling in any significant way, those tiny savings will add up very quickly. |
|
My opinion is that what @JoostK has posted is a cleaner piece of code then what you still have. |
Changed the return of the method in line with suggestions from @JoostK.
|
Just made the change suggested. Thanks. |
Performance enhancement for Str Class (see pull request #1180)
|
@taylorotwell Hi - any chance of getting this merged alongside the identical changes to Str in #1180? Thanks. |
|
Looks like #1204 (the one with the improvements in the |
|
Opps. That's what I mean! |
|
Hello, Any idea if this change will be able to make its way in to the main codebase? or are there further changes/tweaks needed before this is able to happen. Just noticing that a very similar fix to this was included a few weeks back. Thank you, |
|
+1 |
Performance enhancement for HTML Class.
Hello,
We've recently started using laravel for a number of internal web-apps, but have run in to a couple of issues when load testing. The underlying issue being that the config::get method is significantly more expensive than a standard array access. This is compounded by it being invoked a 100-1000 times per page load.
This pull request includes a change we've made to attempt to mitigate this issue, which functions by caching the application.encoding variable within the HTML class. To use one of our apps as an example, for a large form this reduced the number of calls to config::get from 1200+ invocations (72ms) to 125 (9ms).
All core unit-test's pass with this change in place.
Thank you,
Carl
Note: I originally targeted the wrong branch with my pull request ( #1177 ) so have closed it and opened this one instead. Sorry for the inconvenience.