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 iterator-based version of RouteListFilter #940

Merged
merged 1 commit into from
Jul 3, 2024

Conversation

fasaxc
Copy link
Contributor

@fasaxc fasaxc commented Jan 3, 2024

The RouteListFiltered method can be quite inefficient when there are many (say 100k) routes. First all 100k routes are loaded into fragmented byte slices, appended to a slice (which must be grown through several orders of magnitude, requiring a lot of copying), then the whole [][]byte is kept in RAM while all the routes are parsed and returned in a second large slice.

This PR adds a callback-based iterator equivalent: RouteListFilteredIter. RouteListFilteredIter loads one route from the kernel, parses it fully and passes it to the callback, then it moves to the next route. This should be a lot more efficient, especially when the caller is further converting the routes. To avoid copying too much code, RouteListFiltered is now a simple wrapper around RouteListFilteredIter, which should be more efficient due to avoiding creation of the intermediate [][]byte.

Similarly, the main work of NetlinkRequest.Execute is moved to ExecuteIter.

@fasaxc
Copy link
Contributor Author

fasaxc commented Jan 3, 2024

I ran some benchmarks to confirm:

BenchmarkRouteListFilteredOld
BenchmarkRouteListFilteredOld-12    	      12	  99193504 ns/op	164265036 B/op	  329341 allocs/op
BenchmarkRouteListFilteredNew
BenchmarkRouteListFilteredNew-12    	      12	  88368757 ns/op	155487153 B/op	  329313 allocs/op
BenchmarkRouteListIter
BenchmarkRouteListIter-12           	      27	  41802746 ns/op	27340148 B/op	  329285 allocs/op
PASS

BenchmarkRouteListFilteredOld/New compare the normal RouteListFiltered() method; the new approach seems to give a ~10% improvement even when building the output slice. BenchmarkRouteListIter iterates the routes but discards them; it's 2x as fast and reduces RAM usage.

@fasaxc
Copy link
Contributor Author

fasaxc commented Jan 25, 2024

@aboch @vishvananda Any interest in this kind of approach? I think the reduction in GC burden is pretty large if you have a lot of routes and the performance is better whether you use the new API or the old. It also lays the groundwork for adding Iter versions of any other API methods that would benefit.

@fasaxc fasaxc force-pushed the route-list-iter branch 2 times, most recently from 75f8f06 to c63d596 Compare February 1, 2024 11:26
@fasaxc fasaxc force-pushed the route-list-iter branch 2 times, most recently from 166c7ff to 7e59ffa Compare April 9, 2024 09:07
@fasaxc
Copy link
Contributor Author

fasaxc commented Apr 9, 2024

Rebased and squashed onto latest main.

Allows for listing large numbers of routes without
buffering the whole list in memory at once.

Add benchmarks for RouteListFiltered variants.
@fasaxc
Copy link
Contributor Author

fasaxc commented Jun 12, 2024

Rebased again, and looks like the build is now green.

@aboch
Copy link
Collaborator

aboch commented Jul 3, 2024

LGTM

@aboch aboch merged commit b54f850 into vishvananda:main Jul 3, 2024
2 checks passed
@fasaxc
Copy link
Contributor Author

fasaxc commented Jul 4, 2024

Thanks @aboch!

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