Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.6k
Make\Request::get more performant.#12369
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
| @@ -723,7 +723,17 @@ public static function getHttpMethodParameterOverride() | |||
| */ | |||
| public function get($key, $default = null, $deep = false) | |||
| { | |||
| return $this->query->get($key, $this->attributes->get($key, $this->request->get($key, $default, $deep), $deep), $deep); | |||
| $result = $this->query($key, $this, $deep); | |||
| if ($result === $this) { | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I would rather use the opposite conditions and using an early return rather than assigning$result over and over again inside conditions on it (which can be confusing when seeing a list ofif ($result === $this) { conditions)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
We'd still need to store them to a variable, the only difference is the return would be within each conditional. I'd rather have a single return no?
@KorvinSzanto thanks for your pull request. We're always looking for ways to improve performance! Before discussing about the actual code changes, I'd like to ask you if you have some stats about the performance improvement. That would help maintainers to make a better decision. You can use any of the available performance analysis tools, such as Xdebug, XHProf, Blackfire, QafooLabsProfiler, etc. |
@javiereguiluz currently, the fallback to each bag is done by querying the second bag and using the value as default for the first bag. This is totally inefficient algorithm, given that it means you look in all bags all the time instead of stopping the algorithm once you have a value (the code we have would be efficient only if the |
@stof I know that it looks like a performance improvement, but the first rule of application profiling is that you should not trust your instinct but the metrics and profiles. Given that there are a lot of tools available out there, it should be easy to calculate the performance improvement. |
| if ($result === $this) { | ||
| return $default; | ||
| } | ||
| return $result; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I would have written it this way:
if ($this !==$result =$this->query->get($key,$this,$deep)) {return$result;}if ($this !==$result =$this->attributes->get($key,$this,$deep)) {return$result;}if ($this !==$result =$this->request->get($key,$this,$deep)) {return$result;}return$default;
I'm not sure that using |
@Taluu Why should an extra storage object/key be more performant than something that already exists? |
Fyi, the new way is indeed faster (while the old way is a little bit quicker when request matches):http://jsbin.com/baguvo/1/quiet (times are the average of 10 tests with 1000 calls) |
@KorvinSzanto it would be great if you have some time to finish this PR. Here are some of the things that may be necessary:
|
@javiereguiluz, I'd rather close this and let fabpot submit as he'd like it. |
…rvinSzanto, xabbuh)This PR was submitted for the 2.7 branch but it was merged into the 2.3 branch instead (closes#12537).Discussion----------[HttpFoundation] Make Request::get() more performant| Q | A| ------------- | ---| Bug fix? | no| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets || License | MIT| Doc PR |This finishes the work started by@KorvinSzanto in#12369.Commits-------3039935 reformat code as suggested by@fabpotad64223 Fix typo4162713 Make `\Request::get` more performant.
Previously request get ran ALL searches regardless of whether a result was found. Now it only runs the ones required to find the value.