Skip to content
This repository has been archived by the owner on May 4, 2018. It is now read-only.

Windows: update loop time lazily #1526

Open
wants to merge 1 commit into
base: v1.x
Choose a base branch
from
Open

Windows: update loop time lazily #1526

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion include/uv-win.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -315,7 +315,9 @@ RB_HEAD(uv_timer_tree_s, uv_timer_s);
/* The loop's I/O completion port */ \
HANDLE iocp; \
/* The current time according to the event loop. in msecs. */ \
uint64_t time; \
/* Don't access these fields directly. Use uv__loop_time() instead. */ \
uint64_t cached_time; \
int cached_time_is_valid; \
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did you go with 2 attributes instead of a single one like on the first patch?

Copy link
Contributor

Choose a reason for hiding this comment

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

He changed the name of the first attribute so it would reflect its purpose better.

cached_time -> time.

Copy link
Contributor

Choose a reason for hiding this comment

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

No, before that. The original patch used loop->time and it invalidated it by setting it to 0.

Copy link
Author

Choose a reason for hiding this comment

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

That's because in the previous patch we had nuked uv__time_forward, so it was sufficient to set it to 0 to mean 'invalidated'. Then we realized that we needed to bring uv__time_forward back. So we need to store the minimum value for the next readout, even while the cachec time is invalidated.

/* Tail of a single-linked circular queue of pending reqs. If the queue */ \
/* is empty, tail_ is NULL. If there is only one item, */ \
/* tail_->next_req == tail_ */ \
Expand Down
5 changes: 5 additions & 0 deletions src/unix/timer.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,11 @@
#include <limits.h>


uint64_t uv_now(const uv_loop_t* loop) {
return loop->time;
}


static int timer_less_than(const struct heap_node* ha,
const struct heap_node* hb) {
const uv_timer_t* a;
Expand Down
6 changes: 0 additions & 6 deletions src/uv-common.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -391,12 +391,6 @@ void uv_stop(uv_loop_t* loop) {
}


uint64_t uv_now(const uv_loop_t* loop) {
return loop->time;
}



size_t uv__count_bufs(const uv_buf_t bufs[], unsigned int nbufs) {
unsigned int i;
size_t bytes;
Expand Down
7 changes: 2 additions & 5 deletions src/win/core.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,11 +133,8 @@ int uv_loop_init(uv_loop_t* loop) {
if (loop->iocp == NULL)
return uv_translate_sys_error(GetLastError());

/* To prevent uninitialized memory access, loop->time must be intialized
* to zero before calling uv_update_time for the first time.
*/
loop->time = 0;
uv_update_time(loop);
loop->cached_time = 0;
loop->cached_time_is_valid = 0;

QUEUE_INIT(&loop->wq);
QUEUE_INIT(&loop->handle_queue);
Expand Down
1 change: 1 addition & 0 deletions src/win/internal.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -245,6 +245,7 @@ void uv_timer_endgame(uv_loop_t* loop, uv_timer_t* handle);
DWORD uv__next_timeout(const uv_loop_t* loop);
void uv__time_forward(uv_loop_t* loop, uint64_t msecs);
void uv_process_timers(uv_loop_t* loop);
uint64_t uv__loop_time(const uv_loop_t* loop);


/*
Expand Down
36 changes: 28 additions & 8 deletions src/win/timer.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,14 +33,34 @@


void uv_update_time(uv_loop_t* loop) {
uint64_t new_time = uv__hrtime(UV__MILLISEC);
if (new_time > loop->time) {
loop->time = new_time;
}
/* Invalidate the loop time, so that the next invocation of uv__loop_time()
* will actually update it.
*/
loop->cached_time_is_valid = 0;
}

void uv__time_forward(uv_loop_t* loop, uint64_t msecs) {
loop->time += msecs;
loop->cached_time += msecs;
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't this a bit dangerous? We's need to make sure uv__time_forward is called always after updating the time, right?

Copy link
Author

Choose a reason for hiding this comment

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

The purpose of uv__time_forward is to ensure that when we finish polling with a timeout, the timeout value is reflected in the loop time (because as we saw, GetQueuedCompletionStatus can return a tad early). "the timeout value is reflected in the loop time" is equivalent to saying that the difference between the next time the time is read (with uv__loop_time()) and the last time it was read must be at least 'msecs'. This code satisfies this requirement, because cached_time is the loop time as it was read last.

loop->cached_time_is_valid = 0;
}

uint64_t uv__loop_time(const uv_loop_t* loop) {
uv_loop_t* mutable_loop;
uint64_t new_time;
if (!loop->cached_time_is_valid) {
new_time = uv__hrtime(UV__MILLISEC);
mutable_loop = (uv_loop_t*) loop;
if (new_time > mutable_loop->cached_time) {
Copy link
Contributor

Choose a reason for hiding this comment

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

How could this happen?

Copy link
Author

Choose a reason for hiding this comment

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

Same logic as before this change: because of uv__time_forward(). the current time (new_time) could actually be lower than cached_time, if GetQueuedCompletionStatus returned early.

mutable_loop->cached_time = new_time;
}
mutable_loop->cached_time_is_valid = 1;
}

return loop->cached_time;
}

uint64_t uv_now(const uv_loop_t* loop) {
return uv__loop_time(loop);
}


Expand Down Expand Up @@ -104,7 +124,7 @@ int uv_timer_start(uv_timer_t* handle, uv_timer_cb timer_cb, uint64_t timeout,
uv_timer_stop(handle);

handle->timer_cb = timer_cb;
handle->due = get_clamped_due_time(loop->time, timeout);
handle->due = get_clamped_due_time(uv__loop_time(loop), timeout);
handle->repeat = repeat;
uv__handle_start(handle);

Expand Down Expand Up @@ -168,7 +188,7 @@ DWORD uv__next_timeout(const uv_loop_t* loop) {
*/
timer = RB_MIN(uv_timer_tree_s, &((uv_loop_t*)loop)->timers);
if (timer) {
delta = timer->due - loop->time;
delta = timer->due - uv__loop_time(loop);
if (delta >= UINT_MAX - 1) {
/* A timeout value of UINT_MAX means infinite, so that's no good. */
return UINT_MAX - 1;
Expand All @@ -190,7 +210,7 @@ void uv_process_timers(uv_loop_t* loop) {

/* Call timer callbacks */
for (timer = RB_MIN(uv_timer_tree_s, &loop->timers);
timer != NULL && timer->due <= loop->time;
timer != NULL && timer->due <= uv__loop_time(loop);
timer = RB_MIN(uv_timer_tree_s, &loop->timers)) {

uv_timer_stop(timer);
Expand Down