Add shrink_to_fit and compact methods to DelayQueue #4170

b-naber · 2021-10-14T13:02:01Z

This PR introduces shrink_to_fit and compact methods that internally use the newly added functionality in the slab crate. Compact requires us to re-map Keys that were mapped to different indices in the slab, we use a HashMap for this to keep track of the Keys we handed out and the actual Keys that the slab uses. We also have to re-map the Keys that the Wheel uses in its slots, this is performed on each compact call.

b-naber · 2021-10-14T13:03:24Z

tests-build/tests/fail/macros_invalid_input.stderr

@@ -1,3 +1,75 @@
+warning: this attribute can only be applied to a `use` item


Not entirely sure about this, I'd assume we have to remove the attribute. But I decided to keep this in, since I don't really know why we need this attribute in the first place and secondly why it fails.

This file looks entirely unrelated to this change. Why is it here?

I have no idea, could it be that this is just a newly introduced lint in the compiler?

No, I mean, why did you change the macros_invalid_input.stderr file?

I didn't change this manually. This must have happened while running the tests.

That's weird. Can you undo the change?

I did remove that, but isn't it expected that the test suite modifies stderr output? Why is that lint triggered in the first place? Is that's what you find weird here?

ok nvm I understand. The test suite doesn't output what was contained in the stderr file, thats indeed pretty weird.

b-naber · 2021-10-20T20:34:24Z

Why does the security check fail now? I didn't change anything cargo related in the latest push.

Darksonn · 2021-10-21T07:18:35Z

Someone published a security issue for the chrono crate. It will be fixed in #4186.

Darksonn · 2021-10-21T07:24:48Z

tokio-util/src/time/delay_queue.rs

+
+    /// List of keys that we can use to create new keys. See the comment for
+    /// `create_available_keys` for why this is necessary.
+    available_keys: LinkedList<usize>,


Why is this a LinkedList? That seems rather wasteful when it could just be a vector.

Darksonn · 2021-10-21T07:26:16Z

tokio-util/tests/time_delay_queue.rs

+#[tokio::test]
+async fn compact_expire_empty() {
+    time::pause();


Suggested change

#[tokio::test]

async fn compact_expire_empty() {

time::pause();

#[tokio::test(start_paused = true)]

async fn compact_expire_empty() {

b-naber · 2021-10-23T21:17:57Z

@Darksonn Thanks for the review, addressed your comments.

tokio-util/src/time/delay_queue.rs

b-naber · 2021-10-25T15:21:11Z

@Darksonn Addressed your review.

Darksonn · 2021-10-25T17:26:17Z

tokio-util/src/time/delay_queue.rs

+#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)]
+struct KeySlab {
+    index: usize,
+}


Should probably have a short documentation snippet for future readers of the code.

Darksonn · 2021-10-25T17:27:45Z

tokio-util/src/time/delay_queue.rs

+        let mut key = self.slab.insert(Data {
            inner: value,
            when,
            expired: false,
            next: None,
            prev: None,
        });


We should really make sure to wrap these integers in our key types as soon as we possibly can.

Suggested change

let mut key = self.slab.insert(Data {

inner: value,

when,

expired: false,

next: None,

prev: None,

});

let mut key = KeySlab::new(self.slab.insert(Data {

inner: value,

when,

expired: false,

next: None,

prev: None,

}));

Darksonn · 2021-10-25T17:29:52Z

tokio-util/src/time/delay_queue.rs

-        self.insert_idx(when, key);
+        // `old_key` is the actual index the slab uses internally
+        self.insert_idx(when, old_key);


Do these still use raw integers? It makes me feel a lot safer if we use the key-types as widely as we can, because it's just a lot more robust than raw integers.

Darksonn · 2021-10-25T17:31:29Z

tokio-util/src/time/delay_queue.rs

+        let key_map = &self.key_map;
+        let remapped_key = match key_map.get(&*key) {
+            Some(k) => *k,
+            None => (*key).into(),
+        };


Maybe refactor this into a method?

Darksonn · 2021-10-25T17:51:28Z

tokio-util/src/time/delay_queue.rs

+    /// A `compact` call requires a re-mapping of the `Key`s that were changed
+    /// during the `compact` call of the `slab`. Since the keys that were given out
+    /// cannot be changed retroactively we need to keep track of these re-mappings.
+    /// The keys of `key_map` correspond to the old keys that were given out and
+    /// the values to the `Key`s that were re-mapped by the `compact` call.
+    key_map: HashMap<Key, KeySlab>,


You don't remove items from this map when they are removed from the DelayQueue. The following tests therefore fail:

#[tokio::test(start_paused = true)] async fn remove_after_compact() { let now = Instant::now(); let mut queue = DelayQueue::new(); let foo_key = queue.insert_at("foo", now + ms(10)); queue.insert_at("bar", now + ms(20)); queue.remove(&foo_key); queue.compact(); let panic = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| { queue.remove(&foo_key); })); assert!(panic.is_err()); } #[tokio::test(start_paused = true)] async fn remove_after_compact_poll() { let now = Instant::now(); let mut queue = task::spawn(DelayQueue::new()); let foo_key = queue.insert_at("foo", now + ms(10)); queue.insert_at("bar", now + ms(20)); sleep(ms(10)).await; assert_eq!(assert_ready_ok!(poll!(queue)).key(), foo_key); queue.compact(); let panic = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| { queue.remove(&foo_key); })); assert!(panic.is_err()); }

We do remove Keys from key_map here. I haven't tried this out, but it seems to me as if calling remove with a Key that was already removed should also panic in the current master. I think we should return an Option in remove.

To be clear, both of the tests I wrote here should panic in their last remove, i.e. the one I wrapped in catch_unwind. And it seems like the problem they are revealing is actually something different than not removing stuff from the map, namely that using remove with a key that doesn't exist, but which something else is mapped to, behaves incorrectly.

(though poll_idx still doesn't remove from the map as it should - another test would be needed for that case)

b-naber · 2021-10-27T14:50:39Z

@Darksonn Isolated the key routing logic to the slab and fixed the bug revolving around remove.

Darksonn

The code for the delay queue has definitely become a lot simpler.

Darksonn · 2021-10-29T15:42:56Z

tokio-util/Cargo.toml

@@ -45,7 +45,8 @@ futures-io = { version = "0.3.0", optional = true }
 futures-util = { version = "0.3.0", optional = true }
 log = "0.4"
 pin-project-lite = "0.2.0"
-slab = { version = "0.4.1", optional = true } # Backs `DelayQueue`
+slab = { version = "0.4.4", optional = true } # Backs `DelayQueue`
+tracing = "0.1.29"


What's up with this?

tokio-util/src/time/delay_queue.rs

Darksonn · 2021-10-29T15:59:41Z

tokio-util/src/time/delay_queue.rs

+    pub(crate) fn compact(&mut self) {
+        self.compact_called = true;
+


I think the implementation can be simplified a bit.

pub(crate) fn compact(&mut self) { if !self.compact_called { for (key, _) in self.inner.iter() { self.key_map.insert(key, key); } } let mut remapping = HashMap::new(); slab.compact(|_, from, to| { remapping.insert(from, to); true }); // At this point `key_map` contains a mapping for every element. for internal_key in self.key_map.values_mut() { if let Some(new_internal_key) = remapping.get(&*internal_key) { *internal_key = new_internal_key; } } }

Darksonn · 2021-10-29T16:05:09Z

tokio-util/src/time/delay_queue.rs

+    // We maintain a set of available keys for efficiency reasons, so as not to calculate
+    // the smallest available key each time `self.inner.insert` outputs a duplicate key.
+    // The creation of new keys is necessary if the `self.inner.insert` call
+    // in `self.insert` gives back a key that was previously given out.
+    // This scenario of a duplicate key can only happen after `compact` was called.
+    fn create_available_keys(&mut self) {
+        assert!(self.available_keys.is_empty());
+        self.available_keys.reserve(AVAILABLE_KEYS_SET_SIZE);
+
+        let mut i = 0;
+        let mut num_created_keys = 0;
+        while num_created_keys < AVAILABLE_KEYS_SET_SIZE {
+            if !self.key_map.contains_key(&Key::new(i)) {
+                self.available_keys.insert(KeyInternal::new(i));
+                num_created_keys += 1;
+            }
+            i += 1;
+        }
+    }


I'm not a big fan of this. It makes insert run in linear time in some cases, which is not great.

@Darksonn I don't see a better solution to this. We can add removed Keys back into available_keys, this slightly limits the number of create_new_key calls, but when we add many new keys after a compact call then I think we just have to live with a linear runtime every AVAILABLE_KEYS_SET_SIZE insert calls.

I'm not sure if/how bug prone simply incrementing an index to create new Keys would be in practice... I guess not really. But if the key index does overflow this way then the bug is really subtle. We can't really handle an overflow since we then have to check each new Key index again for duplication.

Do you have a solution for this?

Having a counter for the next id seems fine to me if you do it like this:

while key_is_in_use(self.next_key) { self.next_key = self.next_key.wrapping_add(1); } return self.next_key;

Darksonn · 2021-11-09T14:33:26Z

Do you need further review on this from me at this time, or are you able to continue with the PR?

b-naber · 2021-11-12T23:17:04Z

@Darksonn Updated the PR. Thanks for simplifying compact, it's looks a lot nicer now.

b-naber · 2021-12-06T09:42:27Z

@Darksonn Do you want anything else to be changed here?

The lint error here is misplaced imo, I think it perfectly fine to have the if statement in this case. Using get_or_else or entry just leads to borrow checker problems and makes it less straightforward to read. Can we ignore this lint?

Darksonn · 2021-12-06T09:53:10Z

You could write it like this, which doesn't seem too bad?

if let Entry::Occupied(entry) = self.key_map.entry(key.into()) {
    entry.insert(key);
}

I'm not going to review the PR in detail today — this is exam week. Can you remind me again later?

Darksonn · 2021-12-10T12:00:47Z

tokio-util/src/time/delay_queue.rs

+        let remapped_key = match key_map.get(&*key) {
+            Some(k) => *k,
+            None => (*key).into(),
+        };


This appears incorrect. You should only accept the direct mapping if compact has not been called.

tokio-util/src/time/delay_queue.rs

Darksonn · 2021-12-10T12:02:24Z

tokio-util/src/time/delay_queue.rs

+    pub(crate) fn clear(&mut self) {
+        self.inner.clear()


This needs to clear the hash map too. (It could even reset compact_called to false.)

Darksonn · 2021-12-10T12:03:14Z

tokio-util/src/time/delay_queue.rs

+    pub(crate) fn contains(&self, key: &Key) -> bool {
+        let remapped_key = self.remap_key(&key);
+        self.inner.contains(remapped_key.index)
+    }


Due to the thing I mentioned on remap_key, this is incorrect and may return true for items that don't exists.

Darksonn · 2021-12-10T12:04:07Z

tokio-util/src/time/delay_queue.rs

+    fn index(&self, key: Key) -> &Self::Output {
+        let remapped_key = self.remap_key(&key);
+        &self.inner[remapped_key.index]


Darksonn · 2021-12-10T12:05:17Z

tokio-util/src/time/delay_queue.rs

+    /// the slab use [`compact`]
+    /// This function can take O(n) time even when the capacity cannot be reduced or the allocation is
+    /// shrunk in place. Repeated calls run in O(1) though.


If you want a line break, you have to include an empty line in the markdown. (Also, missing period.)

Suggested change

/// the slab use [`compact`]

/// This function can take O(n) time even when the capacity cannot be reduced or the allocation is

/// shrunk in place. Repeated calls run in O(1) though.

/// the slab use [`compact`].

///

/// This function can take O(n) time even when the capacity cannot be reduced or the allocation is

/// shrunk in place. Repeated calls run in O(1) though.

Darksonn · 2021-12-10T12:06:33Z

tokio-util/src/time/delay_queue.rs

+    /// This function is not guaranteed to, and in most cases, won't decrease the capacity of the slab
+    /// to the number of elements still contained in it. To decrease the capacity to the size of
+    /// the slab use [`compact`]


This should be a bit more explicit about why this is.

Darksonn · 2021-12-10T12:07:11Z

tokio-util/src/time/delay_queue.rs

+    /// let key2 = delay_queue.insert(10, Duration::from_secs(10));
+    /// let key3 = delay_queue.insert(15, Duration::from_secs(15));
+    ///
+    /// delay_queue.remove(&key3);


Not removing the last insert seems like a better test.

Suggested change

/// delay_queue.remove(&key3);

/// delay_queue.remove(&key2);

Darksonn · 2021-12-10T12:08:03Z

tokio-util/src/time/wheel/mod.rs

    /// Advances the timer up to the instant represented by `now`.
+    #[instrument(skip(self, store), level = "debug")]
    pub(crate) fn poll(&mut self, now: u64, store: &mut T::Store) -> Option<T::Owned> {
        loop {
+            debug!("inside loop of wheel::poll");


This appears to be testing prints. Please take them out.

b-naber · 2021-12-12T12:46:51Z

You could write it like this, which doesn't seem too bad?
if let Entry::Occupied(entry) = self.key_map.entry(key.into()) {
    entry.insert(key);
}

This isn't what we want semantically though. We actually want to insert a key/value pair with a new key (key_to_give_out), so entry isn't really applicable here.

b-naber · 2021-12-13T20:42:30Z

@Darksonn Any other changes needed?

Darksonn · 2021-12-13T20:52:06Z

It's on my todo-list and I will have another look soon.

b-naber · 2021-12-13T21:18:35Z

Thanks, sorry if the question might have come across as somewhat pushy, it wasn't meant that way.

Darksonn · 2021-12-13T21:25:07Z

No worries. PRs do sometimes gets lost, and it's totally fair to ping me. :)

tokio-util/src/time/delay_queue.rs

Darksonn · 2021-12-14T14:40:45Z

tokio-util/src/time/delay_queue.rs

+    // corresponding internal key. Returns None if there was no compact
+    // call.


This seems wrong. It always returns Some if compact has not been called.

Darksonn · 2021-12-14T14:41:48Z

tokio-util/src/time/delay_queue.rs

+    fn create_new_key(&self) -> KeyInternal {
+        let mut next_key_index = self.next_key_index;
+
+        while self.key_map.contains_key(&Key::new(next_key_index)) {
+            next_key_index = next_key_index.wrapping_add(1);
+        }
+
+        KeyInternal::new(next_key_index)
+    }


You need to remember the value of next_key_index here for the next call.

Darksonn · 2021-12-14T14:47:15Z

tokio-util/src/time/delay_queue.rs

-        if let Some(next) = store[*item].next {
-            store[next].prev = store[*item].prev;
+        if let Some(next) = store[key].next {
+            store[Key::new(next)].prev = store[key].prev;


Seems like the next/prev fields and variables should just be of type Key themselves instead of wrapping them here. We want as little wrapping and unwrapping as possible.

b-naber · 2021-12-28T14:52:23Z

@Darksonn Can you take another look at this, please?

Darksonn · 2021-12-28T15:12:14Z

tokio-util/src/time/delay_queue.rs

 impl<T> wheel::Stack for Stack<T> {
    type Owned = usize;
    type Borrowed = usize;
-    type Store = Slab<Data<T>>;
+    type Store = SlabStorage<T>;


I'm not too familiar with the wheel::Stack trait, so it may not be possible, but shouldn't these be Key as well rather than usize?

b-naber · 2021-12-28T19:03:50Z

@Darksonn fixed that.

b-naber · 2022-01-02T19:30:27Z

@Darksonn Can you take one more look maybe? I'd like to finish this PR soon.

Darksonn

I don't have any other comments.

b-naber · 2022-01-09T11:14:35Z

@Darksonn Just pinging in case you forgot about this. When do you want to merge this?

b-naber commented Oct 14, 2021

View reviewed changes

Darksonn added A-tokio-util Area: The tokio-util crate M-time Module: tokio/time labels Oct 14, 2021

Darksonn reviewed Oct 21, 2021

View reviewed changes

b-naber force-pushed the delayqueue_compact_shrink_to_fit branch from f8ed08e to 1e31107 Compare October 23, 2021 21:17

b-naber force-pushed the delayqueue_compact_shrink_to_fit branch from 1e31107 to 50a4395 Compare October 23, 2021 21:21

Darksonn reviewed Oct 24, 2021

View reviewed changes

tokio-util/src/time/delay_queue.rs Outdated Show resolved Hide resolved

tokio-util/src/time/delay_queue.rs Outdated Show resolved Hide resolved

tokio-util/src/time/delay_queue.rs Outdated Show resolved Hide resolved

tokio-util/src/time/delay_queue.rs Outdated Show resolved Hide resolved

Darksonn reviewed Oct 25, 2021

View reviewed changes

b-naber force-pushed the delayqueue_compact_shrink_to_fit branch from 9646ee3 to 58d9205 Compare October 27, 2021 15:00

Darksonn reviewed Oct 29, 2021

View reviewed changes

Darksonn reviewed Dec 10, 2021

View reviewed changes

b-naber added 8 commits December 12, 2021 21:04

add shrink_to_fit and compact methods to DelayQueue

888938a

address review

18d9da7

address review

f842cb8

address review

6a329f8

move routing logic to SlabStorage and fix bug

9a577d9

get rid of available_keys_list

ab1ada4

simplify compact

ae4ac05

address review

c00d58c

b-naber force-pushed the delayqueue_compact_shrink_to_fit branch from 03a981e to c00d58c Compare December 12, 2021 20:32

Darksonn reviewed Dec 14, 2021

View reviewed changes

address review

e516db4

Darksonn reviewed Dec 28, 2021

View reviewed changes

b-naber added 2 commits December 28, 2021 16:47

change Stack::Owned and Stack::Borrowed to Key

57d7253

clippy

9477d8a

Darksonn approved these changes Jan 2, 2022

View reviewed changes

Darksonn merged commit c800dea into tokio-rs:master Jan 9, 2022

tobz mentioned this pull request Feb 10, 2022

chore: prepare tokio-util 0.7.0 #4486

Merged

		@@ -1,3 +1,75 @@
		warning: this attribute can only be applied to a `use` item

		pub(crate) fn compact(&mut self) {
		self.compact_called = true;

	/// delay_queue.remove(&key3);
	/// delay_queue.remove(&key2);

		// corresponding internal key. Returns None if there was no compact
		// call.

Add shrink_to_fit and compact methods to DelayQueue #4170

Add shrink_to_fit and compact methods to DelayQueue #4170

Conversation

b-naber commented Oct 14, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

b-naber commented Oct 20, 2021

Darksonn commented Oct 21, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

b-naber commented Oct 23, 2021

b-naber commented Oct 25, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

b-naber Oct 25, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

b-naber commented Oct 27, 2021

Darksonn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Darksonn commented Nov 9, 2021

b-naber commented Nov 12, 2021

b-naber commented Dec 6, 2021 • edited Loading

Darksonn commented Dec 6, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

b-naber commented Dec 12, 2021 • edited Loading

b-naber commented Dec 13, 2021

Darksonn commented Dec 13, 2021

b-naber commented Dec 13, 2021

Darksonn commented Dec 13, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

b-naber commented Dec 28, 2021

Choose a reason for hiding this comment

b-naber commented Dec 28, 2021

b-naber commented Jan 2, 2022

Darksonn left a comment

Choose a reason for hiding this comment

b-naber commented Jan 9, 2022

Darksonn commented Oct 21, 2021 •

edited

Loading

b-naber Oct 25, 2021 •

edited

Loading

b-naber commented Dec 6, 2021 •

edited

Loading

b-naber commented Dec 12, 2021 •

edited

Loading