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

DelayQueue::shrink_to_fit #3590

Closed
tikue opened this issue Mar 7, 2021 · 12 comments · Fixed by #4170
Closed

DelayQueue::shrink_to_fit #3590

tikue opened this issue Mar 7, 2021 · 12 comments · Fixed by #4170
Labels
A-tokio-util Area: The tokio-util crate C-feature-request Category: A feature request. E-help-wanted Call for participation: Help is requested to fix this issue. E-medium Call for participation: Experience needed to fix: Medium / intermediate M-time Module: tokio/time

Comments

@tikue
Copy link

tikue commented Mar 7, 2021

For long-lived processes, it'd be nice to be able to reclaim memory from DelayQueue. For example, a server may have short daily spikes in traffic, and after the spike is over, the server should try to lessen its memroy footprint.

It'd be nice if DelayQueue offered a shrink_to_fit method, like HashMap et all have.

Perhaps the implementation of DelayQueue already takes care of this, but it wasn't clear from the documentation, so I figured I'd check!

@tikue tikue added A-tokio Area: The main tokio crate C-feature-request Category: A feature request. labels Mar 7, 2021
@Darksonn Darksonn added A-tokio-util Area: The tokio-util crate M-time Module: tokio/time and removed A-tokio Area: The main tokio crate labels Mar 8, 2021
@Darksonn
Copy link
Contributor

Darksonn commented Mar 8, 2021

This seems like a good idea. One question is how to handle the Slab we have inside it, since its shrink_to_fit method may not be able to shrink it if large keys are in use.

@Darksonn Darksonn added E-help-wanted Call for participation: Help is requested to fix this issue. E-medium Call for participation: Experience needed to fix: Medium / intermediate labels Mar 8, 2021
@aym-v
Copy link

aym-v commented Mar 15, 2021

@Darksonn The soon to be released compact method added by tokio-rs/slab#60 will shrink the slab past its length. This might be a solution but the keys will be changed so a way must be found to match the old keys to the new keys. Another solution might be to completely re-allocate the slab but it will be the same problem with the keys.

@Darksonn
Copy link
Contributor

Yeah, some solution to not break keys is going to be necessary.

@b-naber
Copy link
Contributor

b-naber commented Mar 19, 2021

I'd be interested in working at his. I think we would need a bijective map for the Key <-> usize key values that were changed by the compact call of the slab. We could store a pointer to that map in Data and the closure passed to compact allows us to mutate that map. Is there a problem with using a crate for the bimap (because tokio try to minimize external crate dependency)? Or should we implement this ourselves?

@Darksonn
Copy link
Contributor

I don't want to use a bimap because at that point you might as well just use a HashMap instead of a slab, at which point you can just call HashMap::shrink_to_fit and be done with it.

@b-naber
Copy link
Contributor

b-naber commented Mar 19, 2021

Well but we'd only need to keep the re-mapped keys in the bimap. Unless we somehow modify the Key instances we give out or use these as key values in the wheel implementation, there is no alternative to using a bimap as far as I can tell.

@Darksonn
Copy link
Contributor

Using a HashMap seems like a much better alternative than the bimap to me. I still don't love it though.

@halfzebra
Copy link

Hi @Darksonn!

I'd like to take a stab at this if you agree with replacing Slab with a HashMap.
Maybe there's a better data structure on your mind that would fin this use case?

Let me know what you think!

@Darksonn
Copy link
Contributor

There was some discussion on this on the Tokio discord a few weeks ago. I would encourage you to read it. @b-naber maybe you can summarize?

@halfzebra
Copy link

Thanks for your reply, I think I might have found the discussion you are referring to, it looks like there is progress on this issue.

I'll look into something else 🙂

@b-naber
Copy link
Contributor

b-naber commented Sep 25, 2021

Yes, Ive already implemented this, but haven't had time to fully test it yet. Sorry for not updating the progress here, hope you haven't invested too much time into this yet, @halfzebra.

@halfzebra
Copy link

@b-naber It looks like you've done a pretty good job so far!

Thanks for working on this 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-tokio-util Area: The tokio-util crate C-feature-request Category: A feature request. E-help-wanted Call for participation: Help is requested to fix this issue. E-medium Call for participation: Experience needed to fix: Medium / intermediate M-time Module: tokio/time
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants