Avoid some extra clones/allocations #2865

novacrazy · 2024-08-07T19:16:10Z

Motivation

I noticed when writing a Layer implementation that it requires Clone, and after running a small hello world noticed it cloned a lot. After looking at the code I saw that some of the clones were pointless, just to get a mutable reference that then cloned again internally.

Solution

This adds a couple specialized routines that help avoid a few clones, and cleans up some code elsewhere to avoid a couple more allocations.

The most contentious change might be to MethodRouter::call_with_state, but a match might be more efficient, and the new macro should more easily help catch issues the comment about destructuring was after. Edit: Partially reverted, overlooked something simple.

I'm still looking for places where clones/allocations can be avoided.

pickfire · 2024-08-09T13:53:21Z

@novacrazy I am interested how did you get to figure out where allocations occur. Did you overwrite the global allocator to make it tracks allocation?

novacrazy · 2024-08-09T16:35:15Z

@pickfire I had a hunch it was inefficient somewhere, so I put a println! in my layer/service Clone implementation to confirm. Then I just read through Axum’s code, starting at the Router’s own Service call until I saw all the cloning going on. Nothing special.

When I have some more time I’d like to figure out how to remove even more clones. 1-2 should be possible.

jplatte

Hey, thanks for the PR! I'm not intimately familiar with the code pieces you're changing here. Would you mind splitting this PR into smaller ones? It seems like there's a bunch of independent changes here that would be easier to discuss separately.

novacrazy · 2024-08-09T17:26:59Z

Honestly, I don't think it's necessary to split up. It's only 45 lines changed.

The first change was to get rid of the req.uri().path().to_owned(), which was a pointless allocation. By splitting the request into its parts and body, we can borrow from parts.uri while also having mutable access to the parts.extensions, then just recombine the request when it needs to be sent downstream. Zero cloning.

Second, notice the method oneshot_inner, which calls self.0.get_mut() to acquire mutable access to a mutex's inner value, only to immediately clone it and call oneshot. Instead of get_mut(), we can use into_inner() to simply move the inner value in cases where we have ownership of it, which is what oneshot_inner_owned and call_owned do. Then I replaced some code that cloned and called oneshot_inner/Route::call with those, respectively, since the clone already occurred and we have ownership in those places.

It's also worth noting this old code:

let route = handler.clone().into_route(state);
return RouteFuture::from_future(route.clone().oneshot_inner($req))

which clones the handler, applies state to it, then clones it again for no reason. I fixed that as well.

Lastly, I avoided cloning the Method for use within the call! macro in MethodRouter::call_with_state. Most of the time cloning that is simple, but Method is not Copy, because of custom methods, so may as well just avoid the cloning.

Bmwx12013

Chancha

yanns

This is looking good to me, but I'm no expert here.

yanns · 2024-10-07T09:10:26Z

@novacrazy could you check the conflicts with main?

backport #2865 to main

yanns · 2024-10-09T12:29:32Z

@novacrazy I've created #2968 that would merge your changes with main. I hope this can help you updating to main.

yanns · 2024-10-09T12:30:28Z

axum/src/routing/path_router.rs

                match endpoint {
                    Endpoint::MethodRouter(method_router) => {
                        Ok(method_router.call_with_state(req, state))
                    }
-                    Endpoint::Route(route) => Ok(route.clone().call(req)),
+                    Endpoint::Route(route) => Ok(route.clone().call_owned(req)),


This change is not reflected in any tests. Do you think we could test this?

yanns · 2024-10-09T12:31:32Z

axum/src/routing/route.rs

+    /// Variant of [`Route::call`] that takes ownership of the route to avoid cloning.
+    pub(crate) fn call_owned(self, req: Request<Body>) -> RouteFuture<E> {
+        let req = req.map(Body::new);
+        RouteFuture::from_future(self.oneshot_inner_owned(req))


When comparing with main, it would now be self.oneshot_inner_owned(req).not_top_level().

I'm not sure about the .not_top_level(). And there are no tests checking if this is necessary or not.
Do you think we could test this?

This is a note for me.
The current .not_top_level() is covered by axum/src/routing/tests/mod.rs:1086

yanns · 2024-10-09T12:33:23Z

axum/src/routing/mod.rs

-    fn call_with_state(&mut self, req: Request, state: S) -> RouteFuture<E> {
+    fn call_with_state(self, req: Request, state: S) -> RouteFuture<E> {
        match self {
            Fallback::Default(route) | Fallback::Service(route) => {
-                RouteFuture::from_future(route.oneshot_inner(req))
+                RouteFuture::from_future(route.oneshot_inner_owned(req))
            }
            Fallback::BoxedHandler(handler) => {
-                let mut route = handler.clone().into_route(state);
-                RouteFuture::from_future(route.oneshot_inner(req))
+                let route = handler.into_route(state);
+                RouteFuture::from_future(route.oneshot_inner_owned(req))


There are no tests covering this change. Do you think we could test this?

jplatte · 2024-10-09T12:58:04Z

Given latest comments, I want to emphasize once again that this would be much easier to get merged as several smaller PRs, however small the diff may already be.

novacrazy · 2024-10-09T15:54:49Z

If @yanns wants to take over with #2968 that's fine with me. I ended up just writing my own zero-cost web framework to replace Axum in my stack.

jplatte · 2024-10-09T17:56:49Z

Okay, I'll close this in favor of the new PR then.

This is an extraction of a part of #2865

yanns · 2024-10-09T20:19:03Z

I've extracted one part that is covered by tests in #2969.
I'll tackle the other parts in other PRs, trying to find way to add test coverage.

This is an extraction of a part of #2865

novacrazy added 3 commits August 7, 2024 14:06

Avoid some clones

18448ed

Partially revert method_routing.rs changes

dd0f45c

Fix clone test to reflect improvement

918170a

jplatte reviewed Aug 9, 2024

View reviewed changes

Bmwx12013 suggested changes Aug 10, 2024

View reviewed changes

yanns approved these changes Aug 30, 2024

View reviewed changes

novacrazy requested a review from jplatte August 31, 2024 19:13

mladedav approved these changes Oct 7, 2024

View reviewed changes

yanns added a commit that referenced this pull request Oct 9, 2024

backport PR 2865 to main

88f9c87

backport #2865 to main

yanns mentioned this pull request Oct 9, 2024

backport PR 2865 to main #2968

Closed

yanns reviewed Oct 9, 2024

View reviewed changes

jplatte closed this Oct 9, 2024

yanns added a commit that referenced this pull request Oct 9, 2024

avoid one state clone

2c8cdf7

This is an extraction of a part of #2865

yanns mentioned this pull request Oct 9, 2024

avoid one state clone #2969

Merged

yanns mentioned this pull request Oct 9, 2024

Avoid clone in fallback #2970

Merged

yanns pushed a commit that referenced this pull request Oct 9, 2024

avoid one state clone

43a223f

This is an extraction of a part of #2865

yanns pushed a commit that referenced this pull request Oct 9, 2024

avoid one state clone

b363354

This is an extraction of a part of #2865

This was referenced Oct 10, 2024

avoid cloning the uri/path #2971

Merged

Avoid clone in layer #2973

Merged

cdhowie mentioned this pull request Oct 22, 2024

Provide way to handle a request with a MethodRouter without requiring an exclusive reference #3004

Open

1 task

jplatte pushed a commit that referenced this pull request Nov 14, 2024

Avoid one state clone

0d4e224

This is an extraction of a part of #2865

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Avoid some extra clones/allocations #2865

Avoid some extra clones/allocations #2865

novacrazy commented Aug 7, 2024 •

edited

Loading

pickfire commented Aug 9, 2024

novacrazy commented Aug 9, 2024

jplatte left a comment •

edited

Loading

novacrazy commented Aug 9, 2024 •

edited

Loading

Bmwx12013 left a comment

yanns left a comment

yanns commented Oct 7, 2024

yanns commented Oct 9, 2024

yanns Oct 9, 2024

yanns Oct 9, 2024

yanns Oct 9, 2024

yanns Oct 9, 2024

jplatte commented Oct 9, 2024 •

edited

Loading

novacrazy commented Oct 9, 2024

jplatte commented Oct 9, 2024

yanns commented Oct 9, 2024

Avoid some extra clones/allocations #2865

Avoid some extra clones/allocations #2865

Conversation

novacrazy commented Aug 7, 2024 • edited Loading

Motivation

Solution

pickfire commented Aug 9, 2024

novacrazy commented Aug 9, 2024

jplatte left a comment • edited Loading

Choose a reason for hiding this comment

novacrazy commented Aug 9, 2024 • edited Loading

Bmwx12013 left a comment

Choose a reason for hiding this comment

yanns left a comment

Choose a reason for hiding this comment

yanns commented Oct 7, 2024

yanns commented Oct 9, 2024

yanns Oct 9, 2024

Choose a reason for hiding this comment

yanns Oct 9, 2024

Choose a reason for hiding this comment

yanns Oct 9, 2024

Choose a reason for hiding this comment

yanns Oct 9, 2024

Choose a reason for hiding this comment

jplatte commented Oct 9, 2024 • edited Loading

novacrazy commented Oct 9, 2024

jplatte commented Oct 9, 2024

yanns commented Oct 9, 2024

novacrazy commented Aug 7, 2024 •

edited

Loading

jplatte left a comment •

edited

Loading

novacrazy commented Aug 9, 2024 •

edited

Loading

jplatte commented Oct 9, 2024 •

edited

Loading