Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[HttpKernel] reset kernel start time on reboot#27344
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
[HttpKernel] reset kernel start time on reboot#27344
Uh oh!
There was an error while loading.Please reload this page.
Conversation
| if (true ===$this->booted) { | ||
| if ($this->debug) { | ||
| $this->startTime =microtime(true); | ||
| } |
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 remove the initialization done currently in__clone() as this is now redundant with this one.
Also, to be consistent, what about removing the same code in__construct() and always execute the initialization when theboot() method is called?
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.
As a side note, there is a side effect of this change (but this was already the case as of 3.4). Before 3.4, callingboot() a second time was a no-op. As of 3.4, callingboot() a second time re-initialize the Kernel.
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.
@fabpot I fully agree with the first part about removing it from__clone().
My only concern is removing the time generation from__construct() and moving it totally toboot(). In standard installation I would say it's extremely unlikely that someone used the time beforeboot(), but should we care about possible edge-case here?
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.
No, I would say that it does not matter (that is internal stuff anyway). And that gives more consistent numbers between the first request and the following ones.
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.
@fabpot Fixed.
00cae3b tob7feef0Comparenicolas-grekas commentedMay 25, 2018
Thank you@kiler129. |
This PR was squashed before being merged into the 3.4 branch (closes#27344).Discussion----------[HttpKernel] reset kernel start time on reboot| Q | A| ------------- | ---| Branch? | 3.4| Bug fix? | yes| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets |#27319| License | MIT| Doc PR | n/aI created branch from 3.4, since the furthest thing I could find for the reboot feature wasa4fc492 and it originated during stabilization phase of 3.4.ping@nicolas-grekasCommits-------b7feef0 [HttpKernel] reset kernel start time on reboot
| publicfunctionboot() | ||
| { | ||
| if ($this->debug) { | ||
| $this->startTime =microtime(true); |
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.
shouldn't we do this only when we actually do something (i.e. when not booted, or when the booting again without a request stack size) ?
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.
done in9de5014
I created branch from 3.4, since the furthest thing I could find for the reboot feature wasa4fc492 and it originated during stabilization phase of 3.4.
ping@nicolas-grekas