Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.8k
Add support for microseconds in Stopwatch#23223
Add support for microseconds in Stopwatch#23223javiereguiluz wants to merge 12 commits intosymfony:3.4from
Conversation
| * @returnint The time (in milliseconds) | ||
| * @returnfloat The time (in milliseconds) | ||
| */ | ||
| public function getStartTime() |
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.
changing output of this is BC breaker, and for one that like to write in new php in strict mode, it will crash his software.
<?phpdeclare(strict_types=1);functionNON_STRICTLY_TYPEHINTED_SYMFONY_METHOD___getStartTime_org() {return123; }functionNON_STRICTLY_TYPEHINTED_SYMFONY_METHOD___getStartTime_new() {return123.456; }functionUSER_FUNCTION_THAT_CONSUMES_SYMFONY_OUTPUT(int$a) {echo$a;}USER_FUNCTION_THAT_CONSUMES_SYMFONY_OUTPUT(NON_STRICTLY_TYPEHINTED_SYMFONY_METHOD___getStartTime_org());// OKUSER_FUNCTION_THAT_CONSUMES_SYMFONY_OUTPUT(NON_STRICTLY_TYPEHINTED_SYMFONY_METHOD___getStartTime_new());// Fatal error: Uncaught TypeError: Argument 1 passed to USER_FUNCTION_THAT_CONSUMES_SYMFONY_OUTPUT() must be of the type integer, float given
keradus commentedJun 19, 2017
while i do really like the idea, it's bc breaker, as proven |
javiereguiluz commentedJun 19, 2017
@keradus it's a BC break ... that's why I submitted to 3.4 branch ... should I submit it instead to 4.0? |
keradus commentedJun 19, 2017
3.4 is for features, not bc breakers, isn't it ? |
keradus commentedJun 19, 2017
vs
|
fabpot commentedJun 19, 2017
We cannot break BC in 4.0 without a migration path from 3.4. So target master would not change anything. What we need to do instead is to add an argument to those methods to get the value as a float and return a casted int if not. |
keradus commentedJun 19, 2017
that's why I requested future-compat layer :) |
| $this->start =(int)$start; | ||
| $this->end =(int)$end; | ||
| $this->start = $start; | ||
| $this->end = $end; |
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.
Next to the previous mentioned BC break in the issue, no type cast will also be error prone.
javiereguiluz commentedJun 19, 2017
I've made some changes. Please tell me if I'm going in the right direction. Thanks! |
| * @param int|float $end The relative time of the end of the period (in milliseconds) | ||
| */ | ||
| public function __construct($start, $end) | ||
| public function __construct($start, $end /*, $useMicroPrecision = false*/) |
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 need to keep this commented, as it's still optional since you're adding a default value. So it won't break BC. (However you could add thebool typehint commented for 4.0)
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.
Indeed, especially for a constructor method.
fabpot commentedJun 19, 2017
LGTM but as is, it does not really help as the profiling still won't get the more precise time, right? |
javiereguiluz commentedJun 19, 2017
Not sure if this is what was asked for in#18756 ... but the screenshots in the PR are real: now we see details for < 0ms events. |
fabpot commentedJun 19, 2017
How does it work? Where are you passing |
| } | ||
| $this->start = $morePrecision ? $start : (int) $start; | ||
| $this->end = $morePrecision ? $end : (int) $end; |
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.
do we care about detecting invalid inputs, eg arrays ?
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
javiereguiluz commentedJun 19, 2017
@fabpot I understand your comment now. I had forgotten to commit the change that enables more precision 😊 |
| for ($i = 0; $i < $left; ++$i) { | ||
| $index = $stopped + $i; | ||
| $periods[] = new StopwatchPeriod($this->started[$index], $this->getNow()); | ||
| $periods[] = new StopwatchPeriod($this->started[$index], $this->getNow(), true); |
ogizanagiJun 19, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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.
This also makes theStopwatchEvent::getStartTime(),::getEndTime() and::getDuration() returnint|float, right?
So I fear it won't be enough to ensure BC :/ You'll need to be able to configure the$morePrecision flag higher.
Or, wouldn't it be possible to simply add the flag to the three methods mentioned above, to either return an int (default) or a float (more precision):public function getStartTime(/*bool $morePrecision = false*/) instead (and always store floats in the privateStopwatchPeriod properties)?
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.
adding param to method is BC breaker.
you are thinking more about StopWatchPreciseEvent
ogizanagiJun 20, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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.
Indeed, except if the new arg is commented until 4.0, and implementations not having it trigger deprecations.StopWatchPreciseEvent could be a solution, but you still need a simple way to say you want to use it, higher in the Stopwatch API, without affecting existing code, as it may impact other consumers.
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.
If I get you right - if whole param would be commented, then the way to retrieve it would be tofunc_get_args.
Big 👎 from my side to that practices.
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 already use this "practice" for BC reasons in quite a lot of places in Symfony core.
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.
@keradus Seeallfunc_get_arg instances in Symfony. This practice is actually pretty novel in terms of providing new APIs without causing BC breaks.
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.
And those only are the remaining ones in the master branch.
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.
@keradus Usingfunc_get_args/func_num_args() is the only way to have proper upgrade paths when adding an argument to a method.
They can make the code less readable for a period, but that's for making it better at the end, that is what BC layers are all about.
See e.g.
https://github.com/symfony/symfony/blob/3.3/src/Symfony/Bundle/FrameworkBundle/Translation/Translator.php#L61
https://github.com/symfony/symfony/blob/3.3/src/Symfony/Component/HttpFoundation/Request.php#L597
https://github.com/symfony/symfony/blob/3.3/src/Symfony/Component/DependencyInjection/Definition.php#L805
Worst:https://github.com/symfony/symfony/blob/af4703f6f209a9abc387f81b98e00fad46b89a38/src/Symfony/Bundle/SecurityBundle/Security/FirewallMap.php#L25..#L106
All of these have been removed from master already, and I guess we're all happy about what the changes involving those layers will provide.
| * @param bool $morePrecision If true, time is stored as float to keep the original microsecond precision | ||
| */ | ||
| public function __construct($start, $end) | ||
| public function __construct($start, $end, /* bool */ $morePrecision = false) |
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.
bool here is no longer needed
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'm skeptical about this practice of adding commented type hints, especially when it just duplicates the docblock
| $this->start = (int) $start; | ||
| $this->end = (int) $end; | ||
| $this->start =$morePrecision ? $start :(int) $start; | ||
| $this->end =$morePrecision ? $end :(int) $end; |
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.
missing cast to float IMHO
nicolas-grekas left a comment
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.
with minor comments
nicolas-grekas commentedJul 3, 2017
Approval dismissed, sorry :) IF we happen to really want to have the "BC" flag, then the implementation is partial: we cannot have these hardcoded |
keradus commentedJul 3, 2017
and that's the bc breaker, and I show how it breaks my code. pleasedo respect SEMVER or officially say project is no longer following it. |
robfrawley commentedJul 3, 2017
@keradus The "BC break" is limited to use of |
keradus commentedJul 3, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
not, it's not only limited to Changing return type in 4.0 is fine, as 4.0 is not yet released and 4.0 is dedicated place for BC breakers. http://symfony.com/doc/current/contributing/code/bc.html
|
robfrawley commentedJul 3, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
|
stof commentedJul 3, 2017
@robfrawley assertion could be on the argument of one of your methods, and you could be using the output of the component as input for your code. |
keradus commentedJul 3, 2017
@robfrawley , exactly as@stof said |
javiereguiluz commentedJul 6, 2017
I've updated this PR to follow@ogizanagi's comments. |
ogizanagi left a comment
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.
The collectors should be updated to use the new flag when using getters.
| ----- | ||
| * added the `Stopwatch::reset()` method | ||
| * allowed to measure sub-millisecond times by introducing a third argument to |
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.
The changelog entry needs to be updated too :)
| } | ||
| $this->periods[] = new StopwatchPeriod(array_pop($this->started), $this->getNow()); | ||
| $this->periods[] = new StopwatchPeriod(array_pop($this->started), $this->getNow(), 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.
The third argument is not used anymore (same below)
| { | ||
| $this->start =(int)$start; | ||
| $this->end =(int)$end; | ||
| $this->start = $start; |
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 it be casted to float here to avoid issues with strict types ? (otherwise getters might return an int even with the$morePrecision flag totrue)
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.
yep. and currently, passing eg strings will cause odd working of method, while previously they would be converted in constructor already
javiereguiluz commentedJul 6, 2017
After discussing it with@nicolas-grekas, we're proposing a different implementation for this feature. |
ogizanagi commentedJul 6, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
The issue I see with this new implementation over the flag in getters is that you'll probably update the |
fabpot commentedJul 6, 2017
Thank you@javiereguiluz. |
This PR was squashed before being merged into the 3.4 branch (closes#23223).Discussion----------Add support for microseconds in Stopwatch| Q | A| ------------- | ---| Branch? | 3.4| Bug fix? | no| New feature? | yes| BC breaks? | no?| Deprecations? | no| Tests pass? | yes| Fixed tickets |#18756| License | MIT| Doc PR | -Stopwatch component already supports microseconds (and nanoseconds, etc.) ... but the problem is that it converts the millisecond times to integers, so anything smaller to 1 millisecond is lost. This PR changes that to fix issues like the one explained in#18756.### Before### AfterCommits-------0db8d7f Add support for microseconds in Stopwatch
keradus commentedJul 6, 2017
Great to see this without BC breakers ;) |
…h (javiereguiluz)This PR was squashed before being merged into the 3.4 branch (closes#8146).Discussion----------Added a tip about the support of microseconds in StopwatchThis documentssymfony/symfony#23223. See alsohttps://symfony.com/blog/new-in-symfony-3-4-stopwatch-improvements.Commits-------24718a6 Added a tip about the support of microseconds in Stopwatch
Stopwatch component already supports microseconds (and nanoseconds, etc.) ... but the problem is that it converts the millisecond times to integers, so anything smaller to 1 millisecond is lost. This PR changes that to fix issues like the one explained in#18756.
Before
After