search

camallo / dkregistry-rs

A pure-Rust asynchronous library for Docker Registry API v2
Apache License 2.0
62 stars 39 forks source link

dkregistry: remove tokio-core, use tokio instead #76

Closed steveej closed 5 years ago

steveej commented 5 years ago

@lucab PTAL and let me know if I'm missing something or if the handle is indeed unused

steveej commented 5 years ago

I did some memory measurements and the following are before and after [1]:

Using tokio_core::reactor::Core: (before)

examples/image.rs:53 'run': 101253 bytes escape scope

Using tokio::runtime::current_thread::Runtime: (after)

examples/image.rs:52 'run': 4384 bytes escape scope

For completeness I reverted the commit which removes the core handle to see if that alone has an effect on leaking memory. The conclusion is that it doesn't have a significant effect but I still think we should remove the handle if we don't need it.

lucab commented 5 years ago

We can remove it, I think it was required by the previous hyper async API. Can we fully drop tokio-core from cargo manifest after this rework?

steveej commented 5 years ago

Can we fully drop tokio-core from cargo manifest after this rework?

After changing the occurrences in all examples etc I believe we can. Since you agree I'm going to proceed and we'll find out for sure.

lucab commented 5 years ago

@steveeJ yes, please. And thanks a lot for the rework!

lucab commented 5 years ago

@steveeJ any plan to finish this? I have some other cincinnati changes that may benefit from it.

lucab commented 5 years ago

I pushed forward the changes in your second commit, using tokio runtime instead and completely dropping tokio-core. Travis is green and LGTM. Please have a last look and merge if ok.

steveej commented 5 years ago

I pushed forward the changes in your second commit, using tokio runtime instead and completely dropping tokio-core. Travis is green and LGTM. Please have a last look and merge if ok.

Nice, thanks for picking this one up! I had another look and couldn't spot any issues.