From 7c287c044cf324f32fb343f293499a41ba416c0f Mon Sep 17 00:00:00 2001 From: Andy Date: Thu, 9 Sep 2021 23:27:33 +0300 Subject: [PATCH 1/4] reader_counted_event_queue init --- rfcs/reader_counted_event_queue.md | 77 ++++++++++++++++++++++++++++++ 1 file changed, 77 insertions(+) create mode 100644 rfcs/reader_counted_event_queue.md diff --git a/rfcs/reader_counted_event_queue.md b/rfcs/reader_counted_event_queue.md new file mode 100644 index 00000000..e38d224c --- /dev/null +++ b/rfcs/reader_counted_event_queue.md @@ -0,0 +1,77 @@ +# Feature Name: `reader_counted_event_queue` + +## Summary + +Eliminate 2 frame event limitation, eliminate need in ManualEventReader. Without performance sacrifice. + +Achieve through integration of https://github.com/tower120/rc_event_queue. + +## Motivation + +Current Event implementation may be considered as logically dangerous, since systems that rely on events may simply + not receive messages. And this could happen silently. ManualEventReader solution ... is not ideal. + +This RFC attempts to solve this problem completely. + +## User-facing explanation + +It looks like, there should be no changes to current state of affairs. +Except that events will survive 2 frames window. + +## Implementation strategy + +[How "reader counted event queue" works.](https://github.com/tower120/rc_event_queue/blob/master/doc/principal-of-operation.md) + +`rc_event_queue` should work in `AUTO_CLEANUP = false` mode. Meaning that we will call `cleanup` manually. +For each EventType - make corresponding EventQueue resource. + +EventType can be non-cloneable. + +For each system that read - make EventReader, and store it as system local resource. Get `EventReader::iter()` before each +system run, pass that Iterator to the system. Alternatively, pass `EventReader`, so user can make iterator himself. + +For each system that write - store `Arc` as system local resource. Write with `EventQueue::extend`. For this, +use `small_vec`, or `vec` as local resource. Provide user with means of calling `EventQueue::clear` - +this additional functionality may come in handy, if user decides to clear event queue, and accessing to previous messages +may be dangerous (Like level reloaded, and resource/entity pointed in event message are no longer valid). + +Maintenance (reclaim memory): +* Each frame - for each `EventQueue` call `cleanup`. +* **[SAFETY MEASURE]** Each Nth frame - check length of each `EventQueue`. If it is too big, truncate first chunks/clear (this will not free memory yet). + Next (this will move all EventReaders to new position immediately): + - Either. On all associated EventReaders should be called `EventReader::iter()` or `update_position()` (EventReaders should be presumably "queried" from systems locals, that read that EventType). + - Either. Each frame, in scheduler, for systems with EventReaders, that not run, `update_position()` should be called. + This is very fast operation. + +If there is time window, where nothing happens, and we just like wait for v-sync, it is a good place to put maintenance there. +Maintenance can be skipped few frames, and sliced between several frames. But I think this should be not necessary, since I expect +maintenance to be very fast. + +## Drawbacks + +Potential implementation complexity of calling EventReaders `update_position()` in maintenance [SAFETY MEASURE] part. +But it is _only_ needed to guarantee that queue will not get 100Mb's len. But if that happened - almost certainly +something wrong with game logic. Maybe we should not try to save the day, and just panic instead? + +## Rationale and alternatives + +It is possible to use [rc_event_queue](https://github.com/tower120/rc_event_queue) without engine integration. +But ergonomic will be much better, having first class support. + +https://github.com/bevyengine/rfcs/pull/17 tries to solve the same problem, but in somewhat different way. I think +[rc_event_queue](https://github.com/tower120/rc_event_queue) - based implementation should be faster in all operations. Since +for read - only atomic read is needed; for write - Mutex used, instead of RwLock; cleanup is faster due to chunk-based nature, and being O(1). +Also, `cleanup` is safe to call during system run, since readers does not lock, and writer locks is short - so it is safe to +have long run systems which could took several frames to finish their run. It is also safe to run multiple writers at once, +along with readers too! _It also can work, without need to manually call `cleanup` (with `AUTO_CLEANUP = true`)._ + +## Unresolved questions + +- Should we truncate queue, or panic on reaching queue's length limit? +- Do we need `clear` functionality? I personally think, yes. + +## Future possibilities + +Maybe it would be nice to have soft and hard queue length limits: +- On reaching soft limit callback/special error event? will be triggered. +- On reaching hard limit event queue will be trimmed to some size. _Or panic?_ \ No newline at end of file From a4c85fceaf42564f4c7769384a6d95d51c7e13c6 Mon Sep 17 00:00:00 2001 From: Andy Date: Thu, 9 Sep 2021 23:29:06 +0300 Subject: [PATCH 2/4] typos --- rfcs/reader_counted_event_queue.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/rfcs/reader_counted_event_queue.md b/rfcs/reader_counted_event_queue.md index e38d224c..16883679 100644 --- a/rfcs/reader_counted_event_queue.md +++ b/rfcs/reader_counted_event_queue.md @@ -39,12 +39,12 @@ Maintenance (reclaim memory): * Each frame - for each `EventQueue` call `cleanup`. * **[SAFETY MEASURE]** Each Nth frame - check length of each `EventQueue`. If it is too big, truncate first chunks/clear (this will not free memory yet). Next (this will move all EventReaders to new position immediately): - - Either. On all associated EventReaders should be called `EventReader::iter()` or `update_position()` (EventReaders should be presumably "queried" from systems locals, that read that EventType). + - Either. On all associated EventReaders should be called `update_position()` (EventReaders should be presumably "queried" from systems locals, that read that EventType). - Either. Each frame, in scheduler, for systems with EventReaders, that not run, `update_position()` should be called. This is very fast operation. If there is time window, where nothing happens, and we just like wait for v-sync, it is a good place to put maintenance there. -Maintenance can be skipped few frames, and sliced between several frames. But I think this should be not necessary, since I expect +Maintenance can be skipped few frames, and sliced between several frames. But I think this should not be necessary, since I expect maintenance to be very fast. ## Drawbacks From d8d3fff76ff0f9d36796545091bbf06087c6ea01 Mon Sep 17 00:00:00 2001 From: Andy Date: Fri, 10 Sep 2021 12:39:38 +0300 Subject: [PATCH 3/4] unsubscription/subscription possibility --- rfcs/reader_counted_event_queue.md | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/rfcs/reader_counted_event_queue.md b/rfcs/reader_counted_event_queue.md index 16883679..7b85ec1a 100644 --- a/rfcs/reader_counted_event_queue.md +++ b/rfcs/reader_counted_event_queue.md @@ -71,7 +71,8 @@ along with readers too! _It also can work, without need to manually call `cleanu - Do we need `clear` functionality? I personally think, yes. ## Future possibilities - -Maybe it would be nice to have soft and hard queue length limits: -- On reaching soft limit callback/special error event? will be triggered. -- On reaching hard limit event queue will be trimmed to some size. _Or panic?_ \ No newline at end of file +* Maybe we should provide user means of [unsubscription/subscription] or pausing subscription(?)? For example, if systems sees that it going down for a +long time/forever, or somehow need to catch event messages only from specific timeframe. +* Maybe it would be nice to have soft and hard queue length limits: + - On reaching soft limit callback/special error event? will be triggered. + - On reaching hard limit event queue will be trimmed to some size. _Or panic?_ \ No newline at end of file From cffbff4767ce100a2ba6bd36f5eed7742b21d533 Mon Sep 17 00:00:00 2001 From: tower120 Date: Wed, 6 Oct 2021 18:49:37 +0300 Subject: [PATCH 4/4] typo update --- rfcs/reader_counted_event_queue.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/rfcs/reader_counted_event_queue.md b/rfcs/reader_counted_event_queue.md index 7b85ec1a..2900598b 100644 --- a/rfcs/reader_counted_event_queue.md +++ b/rfcs/reader_counted_event_queue.md @@ -20,7 +20,7 @@ Except that events will survive 2 frames window. ## Implementation strategy -[How "reader counted event queue" works.](https://github.com/tower120/rc_event_queue/blob/master/doc/principal-of-operation.md) +[How "reader counted event queue" works.](https://github.com/tower120/rc_event_queue/blob/master/doc/principle-of-operation.md) `rc_event_queue` should work in `AUTO_CLEANUP = false` mode. Meaning that we will call `cleanup` manually. For each EventType - make corresponding EventQueue resource. @@ -75,4 +75,4 @@ along with readers too! _It also can work, without need to manually call `cleanu long time/forever, or somehow need to catch event messages only from specific timeframe. * Maybe it would be nice to have soft and hard queue length limits: - On reaching soft limit callback/special error event? will be triggered. - - On reaching hard limit event queue will be trimmed to some size. _Or panic?_ \ No newline at end of file + - On reaching hard limit event queue will be trimmed to some size. _Or panic?_