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

RFC 0012: timers #12

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

RFC 0012: timers #12

wants to merge 3 commits into from

Conversation

ysbaddaden
Copy link
Collaborator

No description provided.

@ysbaddaden ysbaddaden added the rfc label Nov 22, 2024
@ysbaddaden ysbaddaden self-assigned this Nov 22, 2024
@ysbaddaden ysbaddaden changed the title RFC 0000: timers RFC 0012: timers Nov 22, 2024
## Data structure: min pairing heap

A min-heap is a simple, fast and efficient tree data structure, that keeps the
smaller value as the HEAD of the tree (the rest isn't ordered). This is enough
Copy link

Choose a reason for hiding this comment

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

smallest?


- Go stores all timers into a min-heap (4-ary) but allocates timers in the GC
HEAP and merely marks cancelled timers on delete. I didn't investigate how
it deals with the tombstones.
Copy link

@RX14 RX14 Nov 23, 2024

Choose a reason for hiding this comment

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

This sounds worth investigating further to me, I actually wrote a message above which described doing exactly this, but I deleted it when I read this part. Tombstones can probably be kept around until dequeue, though there may be other opportune times to delete them if scanning/moving entries anyway.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It could be interesting to understand, but I wonder about the benefit.

Keeping the tombstones means they might stay for seconds, minutes or hours despite having been cancelled. The occupancy would no longer be how many active timers there are now, but the total number of timers created in the last N seconds/minutes/hours.

They also increase the cost of delete-min: it must be repeated multiple times until we reach a not cancelled timer (not cool).

We'd have to allocate the event in the GC HEAP (we currently allocate events on the stack) and they'd stay allocated until they finally leave the 4-heap.

We can probably clear the tombstones we meet as we swap items (cool), but that means dereferencing each pointer, which reduces the CPU cache benefit of the flat array...

Copy link

@RX14 RX14 Nov 24, 2024

Choose a reason for hiding this comment

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

Yes, the practicalities of the solution might outweigh any performance benefit, but Go going that way provides signal to me it's worth doing the benchmarking.

Comment on lines +67 to +69
In crystal such a `timer` is created when we call `sleep(Time::Span)` or
`Fiber.yield` that behaves as a `sleep(0.seconds)`. There are no public API
to cancel a sleep, and they always expire.
Copy link
Member

Choose a reason for hiding this comment

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

thought: I'm wondering if Fiber.yield should be called as an example of a timer. It's conceptually not a timer, but a concurrency primitive that allows the scheduler to pass execution to a different fiber. The fact that it is equivalent to sleep(0.seconds) is an implementation detail.
We should certainly mention Fiber.yield and its relation to sleep here, of course.

text/0012-timers.md Outdated Show resolved Hide resolved

- high precision (sub-millisecond and below is desireable);
- no need for `delete` (never cancelled);
- more reasonable number of timers (**BOLD CLAIM TO BE VERIFIED**)
Copy link
Member

Choose a reason for hiding this comment

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

thought: I think "reasonable" should be more specific. I assume you mean to expect the number of concurrently active timers to be much smaller than that of timeouts?

Co-authored-by: Johannes Müller <[email protected]>
@straight-shoota straight-shoota marked this pull request as ready for review November 24, 2024 10:58
straight-shoota added a commit to crystal-lang/crystal that referenced this pull request Nov 26, 2024
Related to [RFC #12](crystal-lang/rfcs#12).

Replaces the `Deque` used in #14996 for a min [Pairing Heap] which is a kind of [Mergeable Heap] and is one of the best performing heap in practical tests when arbitrary deletions are required (think cancelling a timeout), otherwise a D-ary Heap (e.g. 4-heap) will usually perform better. See the [A Nearly-Tight Analysis of Multipass Pairing Heaps](https://epubs.siam.org/doi/epdf/10.1137/1.9781611973068.52) paper or the Wikipedia page for more details.

The implementation itself is based on the [Pairing Heaps: Experiments and Analysis](https://dl.acm.org/doi/pdf/10.1145/214748.214759) paper, and merely implements a recursive twopass algorithm (the auxiliary twopass might perform even better). The `Crystal::PointerPairingList(T)` type is generic and relies on intrusive nodes (the links are into `T`) to avoid extra allocations for the nodes (same as `Crystal::PointerLinkedList(T)`). It also requires a `T#heap_compare` method, so we can use the same type for a min or max heap, or to build a more complex comparison.

Note: I also tried a 4-heap, and while it performs very well and only needs a flat array, the arbitrary deletion (e.g. cancelling timeout) needs a linear scan and its performance quickly plummets, even at low occupancy, and becomes painfully slow at higher occupancy (tens of microseconds on _each_ delete, while the pairing heap does it in tens of nanoseconds).

Follow up to #14996 

[Mergeable Heap]: https://en.wikipedia.org/wiki/Mergeable_heap
[Pairing Heap]: https://en.wikipedia.org/wiki/Pairing_heap
[D-ary Heap]: https://en.wikipedia.org/wiki/D-ary_heap

Co-authored-by: Linus Sellberg <[email protected]>
Co-authored-by: Johannes Müller <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants