Windows: update loop time lazily

[orangemocha]

We call uv_update_time() whenever we think the loop time should be updated.
But we might not need to read the time after every update, and the update
operation has non-negligible cost.
So I am making uv_update_time() invalidate the loop time. The time will be
actually updated only on the first call to uv_now() after it's been
invalidated.

[orangemocha]

cc @saghul , @piscisaureus
This is a follow up to #1165
If you don't like the use of uv_now() I could add an internal uv__time(loop_t*) and have uv_now() call that.

[saghul]

If you don't like the use of uv_now() I could add an internal uv__time(loop_t*) and have uv_now() call that.

I think I'd like that, actually :-)

[orangemocha]

@txdv : to signify that it's private

[saghul]

@orangemocha we don't really use that convention in libuv, basically everything except the data field in a loop is considered private. "time" is fine.

[txdv]

Also, note the "txdv commented on 9c100d5 include/uv-win.h:L319 3 hours ago", you can derive by that that I commented on a specific line of code.

You can click on that to get to that line and with comment on my comment on my comment directly, this way the conversation stays nicely formatted.

[orangemocha]

@saghul That seems a little bug-prone. The unix side is using loop->time directly. I am worried about people adding some common code that accesses loop->time directly. I'd like them to get a compile error on Windows.
If you don't like the underscore, can I change it to something like 'cached_time'?

[txdv]

Will you provide a unix patch?

[orangemocha]

Currently, I am leaving the unix side as is. I am debating whether I should introduce uv__time() for unix as well. Let me publish the current change so you can see what I mean. Thanks!

[orangemocha]

updated

[txdv]

@saghul Didn't we agree to prefix internal functions with uv__? (double _)

uv_update_time would be a perfect target for this.

[saghul]

uv__loop_time maybe?

[orangemocha]

sure

[saghul]

@saghul Didn't we agree to prefix internal functions with uv__? (double _)

uv_update_time would be a perfect target for this.

Yes.

[txdv]

uv_update_time is not internal, I assumed for a moment that it was. Forget my stupid comment.

[saghul]

Quick (perhaps stupid) idea: get rid of uv_update_time and modify uv_now (remove constness + docs) to do it. Also on Unix.

[txdv]

To do what exactly?

[saghul]

To do what exactly?

To do what uv_update_time currently does.

[txdv]

Right now the time is being updated at the beginning of the event loop. Then you can call uv_now to get that 'cached' time.

Now if you would have long(er) operation, you might want to update the time manually, which is why uv_update_time exists. You call it and then uv_now returns the new most currently cached time.

Now if you merge uv_update_time and uv_now, and call uv_now a lot of times in a row, it will be reduce performance.

The existence of uv_update_time lets us be as fast as possible in most situations while letting us be as accurate as possible when the developer invokes uv_update_time. I would like to merge that into one function as well, but I don't think that is possible.

This makes me just realize that making uv_now lazy is kinda useless, because when the developer invokes uv_update_time he really means to update it now.

[saghul]

uv_now is pretty useless, since it returns the time perception of the loop, if you want high precission time there is uv_hrtime for that. I seriously doubt anybody uses uv_now and would be hit by this, though I could be wrong.

[orangemocha]

I agree with @txdv:

The existence of uv_update_time lets us be as fast as possible in most situations while letting us be as accurate as possible when the developer invokes uv_update_time.

uv_update_time is cheap to call and we call it any time we know the time might have changed. If we didn't use it, we would have to update the time every time we read it, which would be less performant and also change the behavior. It's not uv_now() that we need to worry about, but uv__loop_time(), which is used for timer manipulation.

[orangemocha]

Pushed a new commit with changes from code review feedback above.

I am thinking it might be better to introduce uv__loop_time on Unix as well, to keep a consistent internal interface, even if doesn't do the caching there. What do you think?

[txdv]

Why would someone not call uv_now after he called uv_update_time?

uv_update_time is used internally though ... maybe it might make sense.

[orangemocha]

Correct, uv_update_time is used internally, it seems mostly to support timer operations.

[txdv]

Not is not, it is an exposed function in the uv.h header

[orangemocha]

I meant it's used internally, in addition to being part of the public API. The internal use is for timers and it doesn't imply that uv_now() will be called afterwards.

[txdv]

Yeah, that makes sense in that case. Saves maybe a few time queries.

[saghul]

I meant it's used internally, in addition to being part of the public API. The internal use is for timers and it doesn't imply that uv_now() will be called afterwards.

My point is that uv_now is not designed to be precise. The fact that on Windows is precise is a coincidence. This becomes clearer when looking at the Unix implementation, because it uses different clock sources for uv_now and uv_hrtime: https://github.com/joyent/libuv/blob/master/src/unix/internal.h#L293-L297 and https://github.com/joyent/libuv/blob/master/src/unix/core.c#L89-L91

That said, what I proposed would make uv_now update the loop time + return it, thus "deprecating" uv_update_time. Those who were manually updating the time by calling uv_update_time would just call uv_now. Alternatively we could remove uv_now completely since, as I said earlier, it's pretty useless. Then we'd just leave uv_update_time and there would be no way to fetch the loop's time, which I'd say it's ok.

[saghul]

@piscisaureus mind throwing your 2 cents here?