Skip to content

Do not construct a Vec for evicted LFT flow IDs - #1045

Open
FelixMcFelix wants to merge 3 commits into
uft-ttl-fixfrom
evict-no-vec
Open

Do not construct a Vec for evicted LFT flow IDs#1045
FelixMcFelix wants to merge 3 commits into
uft-ttl-fixfrom
evict-no-vec

Conversation

@FelixMcFelix

@FelixMcFelix FelixMcFelix commented Aug 28, 2026

Copy link
Copy Markdown
Collaborator

When cleaning up a flow table, we have a mechanism to identify all partner flows of removed elements. This is mainly needed for Layers -- the lifetimes of in/out flows are tied together, so entries need to be removed from both tables.

This is done by constructing a Vec of all the partner flow IDs, which we extract using a closure. When we have to expire many entries in one shot, this causes us to spend time reallocating a load of temporary storage for all of their InnerFlowIds as the list builds. But since the closure is a mandatory parameter, this also leads to us also constructing a useless Vec full of FLOW_ID_DEFAULT for the UFT/TCP flow tables.

This change prevents us from creating such a Vec at all in either case, and performs removals from a partner table directly when needed.


pub fn expire_flows<F>(&mut self, now: Moment, f: F) -> Vec<InnerFlowId>
where
pub fn expire_flows(&mut self, now: Moment) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Previously this FlowTable::expire_flows was called by LayerFlowTable::expire_flows which would call FlowTable::expire on each of the returned entries, which would remove it from FlowTable but I guess that was redundant with the self.map.retain below which is also there in the master branch?

I see that expiry_cleanup covers what propogate_last_hit was previously doing in FlowTable::expire, but i'm not seeing a corollary to entry.mark_evicted. Is that ok?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Previously this FlowTable::expire_flows was called by LayerFlowTable::expire_flows which would call FlowTable::expire on each of the returned entries, which would remove it from FlowTable but I guess that was redundant with the self.map.retain below which is also there in the master branch?

I don't think there was anything redundant in the old behaviour. self.ft_out.expire_flows(now, LftOutEntry::extract_pair) produced a list of inbound flow IDs, which we then expired from self.ft_in.

I see that expiry_cleanup covers what propogate_last_hit was previously doing in FlowTable::expire, but i'm not seeing a corollary to entry.mark_evicted. Is that ok?

That's... a good question. It doesn't matter here because we've just asserted that the flow has no children (so mark_evicted is a no-op), and we shouldn't be doing timer expiry while any children depend on the flow (in contrast with eviction). For LFT partner expiry, the lifetime field of the in/out entries are guaranteed to be the same Arc'd element. I'll add some commentary on the function to explain the FlowEntry lifetimes must be entwined like that.

I worry that could break down if we change up the locking model, however. E.g., if we move to layer-wise locks in future. We've already documented that in associate_lfts_upstack, but I'll reiterate some of it here too.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've explained this better now, and made a few further tweaks around when we should (and can avoid double-) calling mark_evicted in certain contexts.

When we have to expire many entries in one shot, this causes us to spend
time reallocating a load of temporary storage for all of their
`InnerFlowId`s. It's better if we _not_ do so and just remove them
directly.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants