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

Add GC & threshold to the ARP cache. #234

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
4 participants
@squidarth
Copy link
Contributor

squidarth commented Jun 12, 2018

Implementation of solution discussed in: #83

Main things to look at:

  1. I figured this only really makes sense for the ManagedMap::Owned case, so I conditionally compile for the alloc case. Let me know if this is wrong

  2. There weren't any tests in the neighbor.rs file that use the Owned case, so I had to start using BTreeMap. I'm not totally sure what the downsides of this are, so let me know if there's anything I should be aware of.

  3. The reason I needed to upgrade the version of managed is so that we could have len operator on ManagedMap.

Manual Testing

I did manually test this with the httpclient example (i changed around where run_gc gets run in order to make it do something) and checked that the sizes were correct.

thanks for taking a look at this!

@squidarth squidarth force-pushed the squidarth:ss-83-arp-cache-eviction branch from c532d8c to 53fd624 Jun 12, 2018

@dlrobertson
Copy link
Collaborator

dlrobertson left a comment

Thanks for working on this

ManagedMap::Borrowed(_) => {
unreachable!()
}
#[cfg(any(feature = "std", feature = "alloc"))]

This comment has been minimized.

@dlrobertson

dlrobertson Jun 13, 2018

Collaborator

This is a duplicate if run_gc is prefixed by the same cfg.

}

#[cfg(any(feature = "std", feature = "alloc"))]
pub fn run_gc(&mut self, timestamp: Instant) {

This comment has been minimized.

@dlrobertson

dlrobertson Jun 13, 2018

Collaborator

Could you make it clearer that this is only run with the Owned variant. IMO a doc comment or changing the name to run_owned_gc would be sufficient.

Cache::new_with_gc_thresh(storage, Cache::GC_THRESH)
}

pub fn new_with_gc_thresh<T>(storage: T, gc_thresh: usize) -> Cache<'a>

This comment has been minimized.

@dlrobertson

dlrobertson Jun 13, 2018

Collaborator

Would with_limit be more readable. 🚲 🏠

}
#[cfg(any(feature = "std", feature = "alloc"))]
ManagedMap::Owned(ref mut map) => {
net_trace!("In the Owned Case");

This comment has been minimized.

@dlrobertson

dlrobertson Jun 13, 2018

Collaborator

Is this necessary? Or was this just used for debugging?

@squidarth squidarth force-pushed the squidarth:ss-83-arp-cache-eviction branch from 53fd624 to d05215f Jun 13, 2018

@squidarth

This comment has been minimized.

Copy link
Contributor Author

squidarth commented Jun 13, 2018

@dlrobertson, awesome, thanks for the review. updated!

@dlrobertson
Copy link
Collaborator

dlrobertson left a comment

LGTM

@squidarth

This comment has been minimized.

Copy link
Contributor Author

squidarth commented Jun 18, 2018

cool! @dlrobertson do you have any ides for other issues that might be good to get started on next? thanks

@@ -51,6 +51,7 @@ pub(crate) enum Answer {
pub struct Cache<'a> {
storage: ManagedMap<'a, IpAddress, Neighbor>,
silent_until: Instant,
gc_thresh: usize

This comment has been minimized.

@whitequark

whitequark Jun 18, 2018

Member

gc_threshold, please

@@ -60,23 +61,59 @@ impl<'a> Cache<'a> {
/// Neighbor entry lifetime, in milliseconds.
pub(crate) const ENTRY_LIFETIME: Duration = Duration { millis: 60_000 };

/// Default number of entries in the cache before GC kicks in
pub(crate) const GC_THRESH: usize = 1024;

This comment has been minimized.

@whitequark

whitequark Jun 18, 2018

Member

also here

let mut storage = storage.into();
storage.clear();

Cache { storage, silent_until: Instant::from_millis(0) }
Cache { storage, gc_thresh: gc_thresh, silent_until: Instant::from_millis(0) }

This comment has been minimized.

@whitequark

whitequark Jun 18, 2018

Member

You can use gc_thresh instead of gc_thresh: gc_thresh.

}

#[cfg(any(feature = "std", feature = "alloc"))]
pub fn run_owned_gc(&mut self, timestamp: Instant) {

This comment has been minimized.

@whitequark

whitequark Jun 18, 2018

Member

Should this really be pub?

unreachable!()
}
ManagedMap::Owned(ref mut map) => {
map.iter_mut()

This comment has been minimized.

@whitequark

whitequark Jun 18, 2018

Member

Maybe .into_iter() so that we don't need to clone values? If the naive form doesn't pass borrow checker, use swap.

}

pub(crate) fn fill(&mut self, protocol_addr: IpAddress, hardware_addr: EthernetAddress,
timestamp: Instant) {
debug_assert!(protocol_addr.is_unicast());
debug_assert!(hardware_addr.is_unicast());

match self.storage {
ManagedMap::Borrowed(_) => {

This comment has been minimized.

@dlrobertson

dlrobertson Jun 18, 2018

Collaborator

nit

ManagedMap::Borrowed(_) => (),
#[cfg(any(feature = "std", feature = "alloc"))]
ManagedMap::Owned(_) => {
if self.storage.len() >= self.gc_thresh {
self.run_owned_gc(timestamp);

This comment has been minimized.

@dlrobertson

dlrobertson Jun 18, 2018

Collaborator

Would it be more readable to have a static method that runs GC on a BTreeMap instead of self? That way we don't have the match with the unreachable statement. Just a thought.

This comment has been minimized.

@whitequark

@squidarth squidarth force-pushed the squidarth:ss-83-arp-cache-eviction branch from d05215f to 5a974dc Jun 18, 2018

@squidarth

This comment has been minimized.

Copy link
Contributor Author

squidarth commented Jun 18, 2018

@whitequark @dlrobertson Updated. The problem with the static btree approach is that you can't assign self.storage inside the match block--I addressed this by using swap, but am not totally sure if this is more readable. Let me know what you think, and thanks again for the review

@dlrobertson

This comment has been minimized.

Copy link
Collaborator

dlrobertson commented Jun 18, 2018

am not totally sure if this is more readable.

I think a doc comment could solve this.

@squidarth squidarth force-pushed the squidarth:ss-83-arp-cache-eviction branch from 5a974dc to d91f114 Jun 18, 2018

@squidarth

This comment has been minimized.

Copy link
Contributor Author

squidarth commented Jun 18, 2018

Ah, realized something with the test failure. This isn't going to work with the case where we are using alloc without std, since the static gc method depends on std (for importing the btreemap).

I also don't like that i have to add let _current_storage_size = self.storage.len(); to the top of the match statement.

Thinking it might make sense to just switch back to the non-static run_owned_gcmethod, what do you think?

@squidarth

This comment has been minimized.

Copy link
Contributor Author

squidarth commented Jun 18, 2018

Here's what I had originally for reference (with updates for the other PR comments here): https://github.com/m-labs/smoltcp/compare/master...squidarth:ss-83-arp-cache-eviction-2?expand=1

@dlrobertson

This comment has been minimized.

Copy link
Collaborator

dlrobertson commented Jun 18, 2018

I also don't like that i have to add let _current_storage_size = self.storage.len(); to the top of the match statement.

IMO adding a cfg to the variable would be better.

Ah, realized something with the test failure. This isn't going to work with the case where we are using alloc without std, since the static gc method depends on std (for importing the btreemap).

I think you have two other alternatives here that would be a bit cleaner.

  1. Now that run_gc is static, I can more readily see you could inline the body of run_gc into the Owned match arm without much of a readability penalty.
  2. BTreeMap and mem::replace are not defined strictly in std. You could use core::mem::replace instead of std::mem::replace and something like the following for BTreeMap.
#[cfg(feature = "std")]
use std::collections::BTreeMap;
#[cfg(all(not(feature = "std"), feature = "alloc"))]
use alloc::btree_map::BTreeMap;

@squidarth squidarth force-pushed the squidarth:ss-83-arp-cache-eviction branch 2 times, most recently from aaded5f to 3fbc626 Jun 18, 2018

@squidarth

This comment has been minimized.

Copy link
Contributor Author

squidarth commented Jun 18, 2018

@dlrobertson Great, updated!

Now that run_gc is static, I can more readily see you could inline the body of run_gc into the Owned match arm without much of a readability penalty.

Yep, and now that I've inlined it, I don't need to import BTreeMap anymore.

.filter(|(_, v)| timestamp < v.expires_at)
.collect();

replace(map, new_btree_map);

This comment has been minimized.

@dlrobertson

dlrobertson Jun 18, 2018

Collaborator

1 last nit: use core::mem so that this becomes mem::replace, then it becomes more clear that core::mem::replace is being used here.

@squidarth

This comment has been minimized.

Copy link
Contributor Author

squidarth commented Jun 18, 2018

👍 sounds good, updated

@dlrobertson
Copy link
Collaborator

dlrobertson left a comment

Thanks! Looks good to me.

@dlrobertson

This comment has been minimized.

Copy link
Collaborator

dlrobertson commented Jun 23, 2018

@whitequark any other feedback for this patch?

@whitequark

This comment has been minimized.

Copy link
Member

whitequark commented Jun 24, 2018

Sorry for the delay on this--looks good! Thanks @squidarth!

@m-labs-homu r+

@m-labs-homu

This comment has been minimized.

Copy link
Collaborator

m-labs-homu commented Jun 24, 2018

📌 Commit 4f783ce has been approved by whitequark

m-labs-homu pushed a commit that referenced this pull request Jun 24, 2018

Add GC & threshold to the ARP cache.
Closes: #234
Approved by: whitequark

m-labs-homu pushed a commit that referenced this pull request Jun 24, 2018

Inline the run_gc method.
Closes: #234
Approved by: whitequark

m-labs-homu pushed a commit that referenced this pull request Jun 24, 2018

Use mem::replace instead.
Closes: #234
Approved by: whitequark
@m-labs-homu

This comment has been minimized.

Copy link
Collaborator

m-labs-homu commented Jun 24, 2018

⌛️ Testing commit 4f783ce with merge 80c8be3...

@m-labs-homu

This comment has been minimized.

Copy link
Collaborator

m-labs-homu commented Jun 24, 2018

💔 Test failed - status-travis

@whitequark

This comment has been minimized.

Copy link
Member

whitequark commented Jun 24, 2018

@m-labs-homu retry

@m-labs-homu

This comment has been minimized.

Copy link
Collaborator

m-labs-homu commented Jun 24, 2018

⌛️ Testing commit 4f783ce with merge d7eb969...

m-labs-homu pushed a commit that referenced this pull request Jun 24, 2018

Add GC & threshold to the ARP cache.
Closes: #234
Approved by: whitequark

m-labs-homu pushed a commit that referenced this pull request Jun 24, 2018

Inline the run_gc method.
Closes: #234
Approved by: whitequark

m-labs-homu pushed a commit that referenced this pull request Jun 24, 2018

Use mem::replace instead.
Closes: #234
Approved by: whitequark
@m-labs-homu

This comment has been minimized.

Copy link
Collaborator

m-labs-homu commented Jun 24, 2018

💔 Test failed - status-travis

@whitequark

This comment has been minimized.

Copy link
Member

whitequark commented Jun 24, 2018

Oh--can you please squash the commits?

@squidarth squidarth force-pushed the squidarth:ss-83-arp-cache-eviction branch from 4f783ce to fba1afd Jun 24, 2018

@squidarth

This comment has been minimized.

Copy link
Contributor Author

squidarth commented Jun 24, 2018

@m-labs-homu retry

@m-labs-homu

This comment has been minimized.

Copy link
Collaborator

m-labs-homu commented Jun 24, 2018

@squidarth: 🔑 Insufficient privileges: not in try users

@dlrobertson

This comment has been minimized.

Copy link
Collaborator

dlrobertson commented Jun 24, 2018

@m-labs-homu r=whitequark

@m-labs-homu

This comment has been minimized.

Copy link
Collaborator

m-labs-homu commented Jun 24, 2018

📌 Commit fba1afd has been approved by whitequark

@m-labs-homu

This comment has been minimized.

Copy link
Collaborator

m-labs-homu commented Jun 24, 2018

⌛️ Testing commit fba1afd with merge d4b3318...

@m-labs-homu

This comment has been minimized.

Copy link
Collaborator

m-labs-homu commented Jun 24, 2018

☀️ Test successful - status-travis
Approved by: whitequark
Pushing d4b3318 to master...

@dlrobertson

This comment has been minimized.

Copy link
Collaborator

dlrobertson commented Jun 24, 2018

@squidarth Thanks for working on this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.