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

Add Loop::now() #208

Merged
merged 3 commits into from Nov 26, 2018
Merged

Add Loop::now() #208

merged 3 commits into from Nov 26, 2018

Conversation

trowski
Copy link
Member

@trowski trowski commented Mar 20, 2018

We frequently use microtime() or time() to keep track of last activity time, update time, etc. These system calls are relatively expensive. Loop::now() updates only on the first call in a tick (or in UvLoop the function uv_now() is used).

* {@inheritdoc}
*/
public function now(): int {
return \uv_now($this->handle);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

uv_now is a monotonic timer and not related to the current real time.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right,that was my intention that will need to be clear through documentation, that Loop::now() is not necessarily wall time, but the loop time in milliseconds.

@kelunik
Copy link
Member

kelunik commented Mar 20, 2018

The event loop should use a monotonic timer for now instead of microtime(). In PHP 7.3 this will be possible without extensions using hrtime(). This doesn't avoid any microtime() calls then.

private $now;

/** @var bool */
private $nowUpdateNeeded = true;
Copy link
Member

@staabm staabm Mar 20, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in the other loop-impls., this defaults to false

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, it should be the same to be consistent, though the initial value doesn't matter in this loop.

@trowski
Copy link
Member Author

trowski commented Mar 20, 2018

@kelunik Yes, I should have stated that Loop::now() is not meant to be wall time, but only a relative loop time. As implemented here it's usually wall time, but we can improve it in 7.3 with hrtime().

*
* @return int
*/
abstract public function now(): int;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a BC break.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ugh… I suppose so. We could add an ExtendedDriver, but I'd rather wait for v3 then.

Copy link
Member

@staabm staabm Mar 25, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about adding the methods in the implementation classes but not in the interface?

Copy link
Member

@kelunik kelunik Mar 25, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@staabm Then it can't be used, because the interface / abstract class doesn't cover it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Users which dont need to abstract the impl and use it at app level could use it.

Inside amp you couldn‘t

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The function is mostly useful in libraries, not in applications, so...

Copy link
Member

@kelunik kelunik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This patch breaks BC in the current form and can't be merged into the current version.

@trowski trowski added the v3.x label Mar 25, 2018
@bwoebi
Copy link
Member

bwoebi commented Mar 27, 2018

Not sure why this needs to be abstract. Can't we just simply use the NativeLoop::now() implementation within Driver and override it within the individual drivers?

NativeLoop::now() should be a good enough fallback for any case, we just have additional guarantees about monotonicity within other Drivers.

v3 then can make it abstract. But for now simply not requiring abstract should be just fine.

@trowski trowski force-pushed the loop-now branch 3 times, most recently from c0dd52f to d9a937a Compare November 25, 2018 17:21
@kelunik kelunik removed the v3.x label Nov 25, 2018
public function __construct()
{
$this->handle = new \EvLoop;
$this->nowOffset = (int) (\microtime(true) * self::MILLISEC_PER_SEC);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd even randomize it, so e.g. * random_int(1, 100) / 100

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

public function __construct()
{
$this->handle = new \EvLoop;
$this->nowOffset = (int) (\microtime(true) * self::MILLISEC_PER_SEC);
$this->now = \random_int(0, $this->nowOffset);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason why $this->now is updated here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Picking a random starting value for $this->now. It was always 0, but this makes the value returned by Loop::now() unpredictable.

@kelunik kelunik merged commit 5889f4e into master Nov 26, 2018
@kelunik kelunik deleted the loop-now branch November 26, 2018 20:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants