Add GC & threshold to the ARP cache. #234

squidarth · 2018-06-12T19:12:46Z

Implementation of solution discussed in: #83

Main things to look at:

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
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.
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!

dlrobertson

Thanks for working on this

dlrobertson · 2018-06-13T12:17:09Z

src/iface/neighbor.rs

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


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

dlrobertson · 2018-06-13T12:19:36Z

src/iface/neighbor.rs

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


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.

dlrobertson · 2018-06-13T12:23:08Z

src/iface/neighbor.rs

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


Would with_limit be more readable. 🚲 🏠

dlrobertson · 2018-06-13T12:24:25Z

src/iface/neighbor.rs

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


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

squidarth · 2018-06-13T13:17:55Z

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

dlrobertson

LGTM

squidarth · 2018-06-18T05:06:08Z

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

whitequark · 2018-06-18T13:33:41Z

src/iface/neighbor.rs

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


gc_threshold, please

whitequark · 2018-06-18T13:33:47Z

src/iface/neighbor.rs

@@ -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;


whitequark · 2018-06-18T13:34:03Z

src/iface/neighbor.rs

        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) }


You can use gc_thresh instead of gc_thresh: gc_thresh.

whitequark · 2018-06-18T13:34:25Z

src/iface/neighbor.rs

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


Should this really be pub?

whitequark · 2018-06-18T13:36:23Z

src/iface/neighbor.rs

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


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

dlrobertson · 2018-06-18T13:35:45Z

src/iface/neighbor.rs

    }

    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(_) =>  {


nit

ManagedMap::Borrowed(_) => (),

dlrobertson · 2018-06-18T13:37:36Z

src/iface/neighbor.rs

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


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.

squidarth · 2018-06-18T15:51:01Z

@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 · 2018-06-18T15:56:21Z

am not totally sure if this is more readable.

I think a doc comment could solve this.

squidarth · 2018-06-18T16:29:08Z

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 · 2018-06-18T16:54:47Z

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 · 2018-06-18T16:59:29Z

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.

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.
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 · 2018-06-18T17:26:34Z

@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.

dlrobertson · 2018-06-18T18:42:45Z

src/iface/neighbor.rs

+                        .filter(|(_, v)| timestamp < v.expires_at)
+                        .collect();
+
+                    replace(map, new_btree_map);


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 · 2018-06-18T19:13:03Z

👍 sounds good, updated

dlrobertson

Thanks! Looks good to me.

dlrobertson · 2018-06-23T14:28:51Z

@whitequark any other feedback for this patch?

whitequark · 2018-06-24T06:09:16Z

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

@m-labs-homu r+

m-labs-homu · 2018-06-24T06:09:18Z

📌 Commit 4f783ce has been approved by whitequark

Closes: #234 Approved by: whitequark

m-labs-homu · 2018-06-24T06:33:31Z

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

m-labs-homu · 2018-06-24T07:01:49Z

💔 Test failed - status-travis

whitequark · 2018-06-24T07:02:36Z

@m-labs-homu retry

m-labs-homu · 2018-06-24T07:02:48Z

⌛ Testing commit 4f783ce with merge d7eb969...

Closes: #234 Approved by: whitequark

m-labs-homu · 2018-06-24T07:24:31Z

💔 Test failed - status-travis

whitequark · 2018-06-24T07:59:29Z

Oh--can you please squash the commits?

squidarth · 2018-06-24T16:24:24Z

@m-labs-homu retry

m-labs-homu · 2018-06-24T16:24:26Z

@squidarth: 🔑 Insufficient privileges: not in try users

dlrobertson · 2018-06-24T18:55:44Z

@m-labs-homu r=whitequark

m-labs-homu · 2018-06-24T18:55:46Z

📌 Commit fba1afd has been approved by whitequark

m-labs-homu · 2018-06-24T18:55:59Z

⌛ Testing commit fba1afd with merge d4b3318...

m-labs-homu · 2018-06-24T19:07:37Z

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

dlrobertson · 2018-06-24T19:08:29Z

@squidarth Thanks for working on this!

squidarth force-pushed the ss-83-arp-cache-eviction branch from c532d8c to 53fd624 Compare June 12, 2018 19:17

dlrobertson reviewed Jun 13, 2018

View reviewed changes

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

dlrobertson reviewed Jun 16, 2018

View reviewed changes

whitequark requested changes Jun 18, 2018

View reviewed changes

dlrobertson reviewed Jun 18, 2018

View reviewed changes

squidarth force-pushed the ss-83-arp-cache-eviction branch from d05215f to 5a974dc Compare June 18, 2018 15:48

squidarth force-pushed the ss-83-arp-cache-eviction branch from 5a974dc to d91f114 Compare June 18, 2018 16:06

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

dlrobertson reviewed Jun 18, 2018

View reviewed changes

dlrobertson approved these changes Jun 18, 2018

View reviewed changes

dlrobertson mentioned this pull request Jun 19, 2018

Smarter NeighborCache state machine. #241

Open

whitequark approved these changes Jun 24, 2018

View reviewed changes

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

Add GC & threshold to the ARP cache.

da71d8d

Closes: #234 Approved by: whitequark

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

Inline the run_gc method.

2216d11

Closes: #234 Approved by: whitequark

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

Use mem::replace instead.

80c8be3

Closes: #234 Approved by: whitequark

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

Add GC & threshold to the ARP cache.

c52f319

Closes: #234 Approved by: whitequark

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

Inline the run_gc method.

df3cd97

Closes: #234 Approved by: whitequark

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

Use mem::replace instead.

d7eb969

Closes: #234 Approved by: whitequark

Add GC & threshold to the ARP cache.

fba1afd

squidarth force-pushed the ss-83-arp-cache-eviction branch from 4f783ce to fba1afd Compare June 24, 2018 15:29

m-labs-homu closed this in d4b3318 Jun 24, 2018

whitequark mentioned this pull request Sep 18, 2018

firmware: Use larger ARP cache m-labs/artiq#1153

Merged

Add GC & threshold to the ARP cache. #234

Add GC & threshold to the ARP cache. #234

Conversation

squidarth commented Jun 12, 2018

Manual Testing

dlrobertson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dlrobertson Jun 13, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

squidarth commented Jun 13, 2018

dlrobertson left a comment

Choose a reason for hiding this comment

squidarth commented Jun 18, 2018

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

squidarth commented Jun 18, 2018

dlrobertson commented Jun 18, 2018

squidarth commented Jun 18, 2018

squidarth commented Jun 18, 2018

dlrobertson commented Jun 18, 2018 • edited Loading

squidarth commented Jun 18, 2018

Choose a reason for hiding this comment

squidarth commented Jun 18, 2018

dlrobertson left a comment

Choose a reason for hiding this comment

dlrobertson commented Jun 23, 2018

whitequark commented Jun 24, 2018

m-labs-homu commented Jun 24, 2018

m-labs-homu commented Jun 24, 2018

m-labs-homu commented Jun 24, 2018

whitequark commented Jun 24, 2018

m-labs-homu commented Jun 24, 2018

m-labs-homu commented Jun 24, 2018

whitequark commented Jun 24, 2018

squidarth commented Jun 24, 2018

m-labs-homu commented Jun 24, 2018

dlrobertson commented Jun 24, 2018

m-labs-homu commented Jun 24, 2018

m-labs-homu commented Jun 24, 2018

m-labs-homu commented Jun 24, 2018

dlrobertson commented Jun 24, 2018

dlrobertson Jun 13, 2018 •

edited

Loading

dlrobertson commented Jun 18, 2018 •

edited

Loading