Ordering guarantees for timers #7450

benschulz · 2015-08-30T00:18:31Z

This is an rough solution to the issue described in #3396. XHRs still do their own thing and an overall clean up is in order. Before I do that, though, I'd really like someone to sign off on the overall idea.

There's one major difference to what jdm layed out #3396: The timers remain with the window/worker and only the earliest expiring one is coordinated with the dedicated timer thread.
That means both the timer thread and the window/worker have to keep track of which timer expires next, which feels a bit wonky. However, the upshot is that there's no need for communication with the timer thread when a pipeline is frozen, thawed or dropped.

Most relvant parts are

the TimerScheduler, which is the new per-constellation timer task and
the ActiveTimers which is what's left on the window/worker side.

highfive · 2015-08-30T00:18:36Z

Warning

These commits modify unsafe code. Please review it carefully!

bors-servo · 2015-08-31T21:48:25Z

☔ The latest upstream changes (presumably #7370) made this pull request unmergeable. Please resolve the merge conflicts.

jdm · 2015-09-02T13:14:54Z

Just to avoid radio silence, I've started looking at this several times but have run out time to wrap my head around all the changes. It's pretty dense, unfortunately. I'm going to try using github's split diff view to see if that makes it any easier.

benschulz · 2015-09-03T16:42:10Z

Oh wow, those diffs are pretty worthless. Apparently github does not do --patience diffs. :( Anyway, I try to aim for the opposite of dense; so please let me know if there's anything I could do to make things more clear.

jdm · 2015-09-18T22:11:32Z

For the record, I've been looking at this since last commenting, and I plan to complete my review this weekend.

jdm · 2015-09-22T15:12:03Z

Thank you so much for doing this work @benschulz! This is really well put together, and I sincerely apologize for the long delay in reviewing it properly.
-S-awaiting-review +S-needs-code-changes

Reviewed 19 of 19 files at r1.
Review status: all files reviewed at latest revision, 17 unresolved discussions, some commit checks failed.

^{components/compositing/timer_scheduler.rs, line 49 [r1] (raw file):}
Let's use let _ = tx.send(()); here; we don't need a loud error if the target thread no longer exists.

^{components/compositing/timer_scheduler.rs, line 52 [r1] (raw file):}
Why the +1?

^{components/compositing/timer_scheduler.rs, line 102 [r1] (raw file):}
This looks like endless recursion to me.

^{components/compositing/timer_scheduler.rs, line 125 [r1] (raw file):}
I'm inclined to merge this with the constructor, rather than having a separate method for it.

^{components/compositing/timer_scheduler.rs, line 151 [r1] (raw file):}
if let Some(ref timer_port) = timer_port

^{components/compositing/timer_scheduler.rs, line 205 [r1] (raw file):}
Let's go ahead and use let _ = chan.send(TimerEvent(source, id)); and ignore the error.

^{components/compositing/timer_scheduler.rs, line 218 [r1] (raw file):}
if let Some(ref timer) = timer {

^{components/script/timers.rs, line 42 [r1] (raw file):}
Document this field? Its purpose is not clear to me without reading the rest of the code.

^{components/script/timers.rs, line 94 [r1] (raw file):}
This looks like endless recursion to me.

^{components/script/timers.rs, line 139 [r1] (raw file):}
Why this behaviour?

^{components/script/timers.rs, line 151 [r1] (raw file):}
This needs to use the previous code that allocated a vector with a particular capacity to avoid reallocation.

^{components/script/timers.rs, line 171 [r1] (raw file):}
What does this mean, exactly? I've filed whatwg/html#179 about clearing up the spec text, but all the browsers I tried don't differentiate between intervals and timeouts.

^{components/script/timers.rs, line 188 [r1] (raw file):}
?

^{components/script/timers.rs, line 191 [r1] (raw file):}
Let's add a debug!("ignoring timer fire event {:?} (expected {:?}").

^{components/script/timers.rs, line 253 [r1] (raw file):}
Instead of this line and the previous if, let's do:

if let Some(timer) = timers.peek() {
}

^{components/script/timers.rs, line 286 [r1] (raw file):}
Let's add a debug!("invalidating expected timer (was {:?}, now {:?})" here to make debugging easier.

^{components/script_traits/lib.rs, line 178 [r1] (raw file):}
to fire timers that are due.

Comments from the review on Reviewable.io

benschulz · 2015-09-22T18:53:06Z

Ha, no problem. I appreciate the amount of time it takes to review code. Especially when it's written by an unexperienced (Rust) developer. I also realize that these PRs are small things the core developers could do in a couple of minutes to hours. Of course, these delays do have a negative impact on motivation, but I don't think that should change anything. If anything I think your priorities are correct as they are. TL;DR: No need to apologize.

Review status: all files reviewed at latest revision, 7 unresolved discussions, some commit checks failed.

^{components/compositing/timer_scheduler.rs, line 52 [r1] (raw file):}
Oh boy. I broke it. due_time, current_time and park_time used to all be in milliseconds. Now it's just park_time. I think I better add the unit to all variables. Hopefully the following (and correct 😉) code is self explanatory(?).

park_time_ms = ((due_time_ns - current_time_ns) / (1000 * 1000) + 1) as u32;

^{components/compositing/timer_scheduler.rs, line 125 [r1] (raw file):}
I thought it would be odd to have the constructor of TimerScheduler return a Sender<TimerEventRequest>. Beyond that, I'm on board. Perhaps I'll name the resulting associated function start…

^{components/script/timers.rs, line 139 [r1] (raw file):}
I didn't want intervals with a duration of 0 hogging the CPU. Rereading the spec, I should probably implement the nesting level. At that point this would take care of itself.

^{components/script/timers.rs, line 151 [r1] (raw file):}
Will do. Out of curiosity, though, is that "just" to avoid the allocations or is something being moved that should not be moved (which would surprise me)?

^{components/script/timers.rs, line 171 [r1] (raw file):}
Frankly I'm a bit surprised that this is common behaviour. But I'll leave it be until whatwg/html#179 is resolved.

^{components/script_traits/lib.rs, line 178 [r1] (raw file):}
I'm not a native English speaker, so I have to ask: Does what I wrote make no sense at all or does it mean something I did not intend it to mean? Or is it just terrible style?

Comments from the review on Reviewable.io

jdm · 2015-09-22T19:22:06Z

Review status: all files reviewed at latest revision, 6 unresolved discussions, some commit checks failed.

^{components/compositing/timer_scheduler.rs, line 52 [r1] (raw file):}
What if we used typed units? We could define Nanoseconds and Milliseconds units using http://doc.servo.org/euclid/length/struct.Length.html to avoid this problem (assuming we define a wrapper for precise_time_ns that returns the proper typed unit).

^{components/compositing/timer_scheduler.rs, line 125 [r1] (raw file):}
Yes, I think that's a good choice, and there's prior art for similar constructors that aren't named new and don't return the associated type.

^{components/script/timers.rs, line 0 [r1] (raw file):}
This is for GC safety - heap values are used by the JS engine and contain a pointer to an element of this vector. If the vector reallocates, it invalidates all of the prior Heap values.

^{components/script_traits/lib.rs, line 0 [r1] (raw file):}
I understood it, but I think that "due [something]" tends to mean something different than "[something] is due". Specifically, the first form appears to relate to the quality of something ("due process", "due care") rather than something that is expected at a certain time ("payment is due").

Comments from the review on Reviewable.io

benschulz · 2015-09-24T20:22:52Z

Review status: 15 of 19 files reviewed at latest revision, 4 unresolved discussions, some commit checks failed.

^{components/compositing/timer_scheduler.rs, line 52 [r1] (raw file):}
That's definitely a great idea. I'll put the code in script_traits for now, but that seems wrong. Is there a better place for it?

^{components/script/timers.rs, line 142 [r1] (raw file):}
This is still open. I'll get to it on the weekend.

^{components/script/timers.rs, line 154 [r1] (raw file):}
I still have to wrap my head around this. From what I can see, pointers are getting copied from one Vec to another in both cases; except in one there's a stopover in another Vec.

I'll stare at it some more on the weekend, hopefully I'll grok it then.

Comments from the review on Reviewable.io

jdm · 2015-09-24T20:47:31Z

Review status: 15 of 19 files reviewed at latest revision, 4 unresolved discussions, some commit checks failed.

^{components/compositing/timer_scheduler.rs, line 52 [r1] (raw file):}
Probably utils/ if anywhere, but that's an unfortunate dumping ground. script_traits is fine until we need to use it elsewhere.

^{components/script/timers.rs, line 0 [r1] (raw file):}
The difference is that we construct the Heap values in-place in the preallocated vector in the old code, and it's the Heap constructor that stores a pointer. In the new code, any Heap values in the vector when it reallocates will become invalid.

Comments from the review on Reviewable.io

jdm · 2015-09-24T20:54:26Z

-S-awaiting-review +S-needs-code-changes

Reviewed 4 of 4 files at r2.
Review status: all files reviewed at latest revision, 5 unresolved discussions, some commit checks failed.

^{components/compositing/timer_scheduler.rs, line 123 [r2] (raw file):}
What's the & doing here?

Comments from the review on Reviewable.io

benschulz · 2015-09-27T14:51:02Z

Review status: 10 of 19 files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.

^{components/script/timers.rs, line 142 [r1] (raw file):}
Done.

^{components/script_traits/lib.rs, line 214 [r11] (raw file):}
Sorry, I was too trigger-happy. I need a precise_time_ns version to make sure that the time between set_timeout_or_interval and the callback invocation is at least timeout ms.

^{components/util/mem.rs, line 209 [r9] (raw file):}
I don't feel too good about this. Should I switch to sorted vectors and get rid of the BinaryHeap impls for HeapSizeOf and JSTraceable?

Comments from the review on Reviewable.io

jdm · 2015-10-08T04:35:18Z

Once again, apologies for the delay; I've been stretched really thin recently. I think we're very close to finishing this work, however!
-S-awaiting-review +S-needs-code-changes

Reviewed 2 of 2 files at r3, 1 of 1 files at r4, 1 of 1 files at r5, 1 of 1 files at r6, 1 of 1 files at r7, 6 of 6 files at r8, 1 of 1 files at r9, 2 of 2 files at r10, 1 of 1 files at r11.
Review status: all files reviewed at latest revision, 5 unresolved discussions, some commit checks failed.

^{components/script/timers.rs, line 221 [r11] (raw file):}
I think it would be valuable to add comments like // step 6 in relevant parts of these changes (and add a comment linking to https://html.spec.whatwg.org/multipage/#timer-initialisation-steps above the relevant methods)

^{components/script/timers.rs, line 241 [r11] (raw file):}
FYI: this behaviour seems weird to me but my reading of the spec agrees. I've filed whatwg/html#239 to sort out if we actually want to do this.

^{components/script/timers.rs, line 299 [r11] (raw file):}
Let's call this clamp_duration instead. Also, let's add a spec link to https://html.spec.whatwg.org/multipage/#timer-initialisation-steps step 7

^{components/servo/Cargo.lock, line 1412 [r8] (raw file):}
You'll need to run ./mach cargo-update -p time --precise 0.1.32 to force the other Cargo.lock files to be updated too.

^{components/util/mem.rs, line 209 [r9] (raw file):}
Sure, let's use a sorted vector instead.

Comments from the review on Reviewable.io

benschulz · 2015-10-21T04:52:32Z

Woohoo!

Premature celebration… that was tempting fate. :P

Repeating timers could not clear themselves, because their Timer wasn't in the vector of active timers at the time of execution. I stupidly squashed the fix and it's not getting picked up by reviewable, sorry. :(

jdm · 2015-10-21T13:59:30Z

@bors-servo: r+

Reviewed 1 of 1 files at r15, 20 of 20 files at r17.
Review status: all files reviewed at latest revision, 1 unresolved discussion, all commit checks successful.

Comments from the review on Reviewable.io

bors-servo · 2015-10-21T13:59:31Z

📌 Commit 86063fe has been approved by jdm

bors-servo · 2015-10-21T14:32:35Z

🔒 Merge conflict


        Timers are scheduled by a dedicated per-constellation thread.

jdm · 2015-10-21T14:59:01Z

@bors-servo: r+

bors-servo · 2015-10-21T14:59:02Z

📌 Commit 553a0db has been approved by jdm

bors-servo · 2015-10-21T15:07:31Z

⌛ Testing commit 553a0db with merge 2de5407...


        Auto merge of #7450 - benschulz:constellation-timer, r=jdm

Ordering guarantees for timers This is an rough solution to the issue described in #3396. XHRs still do their own thing and an overall clean up is in order. Before I do that, though, I'd really like someone to sign off on the overall idea. There's one major difference to what jdm layed out #3396: The timers remain with the window/worker and only the earliest expiring one is coordinated with the dedicated timer thread. That means both the timer thread and the window/worker have to keep track of which timer expires next, which feels a bit wonky. However, the upshot is that there's no need for communication with the timer thread when a pipeline is frozen, thawed or dropped. Most relvant parts are - the [`TimerScheduler`](6f5f661#diff-74137a6f50ab38e7a1e4d16920a66ce7R73), which is the new per-constellation timer task and - the [`ActiveTimers`](6f5f661#diff-86707d952414a2860b78bcf6c1db8e2eR34) which is what's left on the window/worker side.  [<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/7450)

bors-servo · 2015-10-21T15:28:06Z

💔 Test failed - mac-dev-ref-unit

jdm · 2015-10-21T15:38:31Z

@bors-servo: retry

#7785

bors-servo · 2015-10-21T16:10:15Z

⚡ Previous build results for android, gonk, linux-dev, linux-rel, mac-rel-css, mac-rel-wpt are reusable. Rebuilding only mac-dev-ref-unit...

bors-servo · 2015-10-21T16:44:12Z

☀️ Test successful - android, gonk, linux-dev, linux-rel, mac-dev-ref-unit, mac-rel-css, mac-rel-wpt

waddlesplash · 2015-10-21T19:32:55Z

Woohoo! 🎊 🎆 🎊 🎆

frewsxcv · 2015-11-04T04:27:07Z

The changes in this pull request caused a regression: #8324

highfive added the S-awaiting-review label Aug 30, 2015

highfive added S-needs-rebase and removed S-awaiting-merge labels Aug 31, 2015

jdm self-assigned this Sep 1, 2015

jdm added S-needs-code-changes and removed S-awaiting-review labels Sep 22, 2015

highfive added S-awaiting-review and removed S-needs-code-changes labels Sep 24, 2015

jdm added S-needs-code-changes and removed S-awaiting-review labels Sep 24, 2015

highfive added S-awaiting-review and removed S-needs-code-changes labels Sep 27, 2015

benschulz force-pushed the benschulz:constellation-timer branch from 5ba0e0e to 169df36 Sep 27, 2015

jdm added S-needs-code-changes and removed S-awaiting-review labels Oct 8, 2015

highfive added S-awaiting-review and removed S-needs-code-changes labels Oct 20, 2015

highfive added S-awaiting-merge and removed S-awaiting-review labels Oct 21, 2015

Timers are scheduled by a dedicated per-constellation thread.

Loading status checks…

553a0db

jdm added S-needs-rebase and removed S-awaiting-merge labels Oct 21, 2015

benschulz force-pushed the benschulz:constellation-timer branch from 86063fe to 553a0db Oct 21, 2015

highfive added the S-awaiting-review label Oct 21, 2015

highfive added S-awaiting-merge and removed S-awaiting-review S-needs-rebase labels Oct 21, 2015

highfive added S-tests-failed and removed S-awaiting-merge labels Oct 21, 2015

bors-servo merged commit 553a0db into servo:master Oct 21, 2015
2 checks passed

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

homu Test successful
Details

benschulz deleted the benschulz:constellation-timer branch Oct 22, 2015

benschulz mentioned this pull request Nov 10, 2015

XHR timeouts use same abstraction as scripts timers. (fixes #3396) #8168

Merged

SimonSapin removed the S-tests-failed label Jan 27, 2016

servo / servo

Ordering guarantees for timers #7450

Ordering guarantees for timers #7450

benschulz commented Aug 30, 2015

highfive commented Aug 30, 2015

bors-servo commented Aug 31, 2015

jdm commented Sep 2, 2015

benschulz commented Sep 3, 2015

jdm commented Sep 18, 2015

jdm commented Sep 22, 2015

benschulz commented Sep 22, 2015

jdm commented Sep 22, 2015

benschulz commented Sep 24, 2015

jdm commented Sep 24, 2015

jdm commented Sep 24, 2015

benschulz commented Sep 27, 2015

jdm commented Oct 8, 2015

benschulz commented Oct 21, 2015

jdm commented Oct 21, 2015

bors-servo commented Oct 21, 2015

bors-servo commented Oct 21, 2015

jdm commented Oct 21, 2015

bors-servo commented Oct 21, 2015

bors-servo commented Oct 21, 2015

bors-servo commented Oct 21, 2015

jdm commented Oct 21, 2015

bors-servo commented Oct 21, 2015

bors-servo commented Oct 21, 2015

waddlesplash commented Oct 21, 2015

frewsxcv commented Nov 4, 2015

servo / servo

Join GitHub today

Ordering guarantees for timers #7450

Ordering guarantees for timers #7450

Conversation

benschulz commented Aug 30, 2015

highfive commented Aug 30, 2015

bors-servo commented Aug 31, 2015

jdm commented Sep 2, 2015

benschulz commented Sep 3, 2015

jdm commented Sep 18, 2015

jdm commented Sep 22, 2015

benschulz commented Sep 22, 2015

jdm commented Sep 22, 2015

benschulz commented Sep 24, 2015

jdm commented Sep 24, 2015

jdm commented Sep 24, 2015

benschulz commented Sep 27, 2015

jdm commented Oct 8, 2015

benschulz commented Oct 21, 2015

jdm commented Oct 21, 2015

bors-servo commented Oct 21, 2015

bors-servo commented Oct 21, 2015

jdm commented Oct 21, 2015

bors-servo commented Oct 21, 2015

bors-servo commented Oct 21, 2015

bors-servo commented Oct 21, 2015

jdm commented Oct 21, 2015

bors-servo commented Oct 21, 2015

bors-servo commented Oct 21, 2015

waddlesplash commented Oct 21, 2015

frewsxcv commented Nov 4, 2015