Skip to content

Conversation

@yanns
Copy link
Collaborator

@yanns yanns commented Oct 10, 2024

extract of #2865

@yanns yanns force-pushed the avoid_cloning_uri_path branch from 47e0831 to c618dab Compare October 10, 2024 09:45
Copy link
Collaborator

@mladedav mladedav left a comment

Choose a reason for hiding this comment

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

Looks good.

Copy link
Member

@jplatte jplatte left a comment

Choose a reason for hiding this comment

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

I wonder whether we should run any benchmarking on this. In theory this could regress things as the request parts are moved around more (into_parts, from_parts).

@yanns
Copy link
Collaborator Author

yanns commented Oct 10, 2024

I wonder whether we should run any benchmarking on this. In theory this could regress things as the request parts are moved around more (into_parts, from_parts).

I don't think this could regress things.

let (mut parts, body) = req.into_parts(); is just a destructure of the Request:

    #[inline]
    pub fn into_parts(self) -> (Parts, T) {
        (self.head, self.body)
    }

and Request::from_parts(parts, body) is in the other direction:

    #[inline]
    pub fn from_parts(parts: Parts, body: T) -> Request<T> {
        Request { head: parts, body }
    }

Both should be no-op.

@jplatte
Copy link
Member

jplatte commented Oct 10, 2024

Both will involve moving things around on the stack, which is not a no-op. In many cases such moves are optimized out when compiling with optimizations, but it's not guaranteed.

@yanns
Copy link
Collaborator Author

yanns commented Oct 10, 2024

I don't have the expertise for this.
I was assuming that destructuring a tuple for example would be no-op.
And I'm assuming that avoiding duplicating the uri path is worth it to avoid allocations.

@jplatte
Copy link
Member

jplatte commented Oct 10, 2024

Okay, I'll look into it a bit later, if time permits. I think we have some existing benchmarks that I can run, probably need to add some more to distinguish short / long paths.

@jplatte
Copy link
Member

jplatte commented Oct 10, 2024

Ran benchmarks locally, the most pronounced result was an improvement for the only test that has a >8 byte long path.

Mixed results for other benchmarks, likely just noise.

@yanns yanns merged commit 1e118cc into main Oct 10, 2024
18 checks passed
@yanns yanns deleted the avoid_cloning_uri_path branch October 10, 2024 20:28
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.

5 participants