perf: Box::pin removed for future returned by self.client_server_exchange(context) - #1396
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
248b4bc to
4096472
Compare
ygxio
changed the title
self.client_server_exchange(context)
ygxio
changed the title
self.client_server_exchange(context)self.client_server_exchange(context)
|
Can i get a review here @levkk |
jkaczman
left a comment
jkaczman
left a comment
There was a problem hiding this comment.
I looked through the issue linked, and it doesn't look like the stats you showed correlate to these exact changes (had some other things included); could you please clarify on the exact improvements here?
|
yes as per keryl i removed the changes of DashMap and only kept the pinning changes |
I saw you had this chart for DashMap + Box::pin, Could you make one for this one solo? |
|
sure it was same for only pin changes, would you mind testing tps on ur system too? |
|
I'm not sure this is the right move. The whole point of this change was to avoid having it on the stack because the future size becomes too large otherwise. Removing this allocation is just going to end up moving the cost up the stack, not get rid of it |
|
@sgrif yes that is right but what i saw is extra rjmallocs, i donot have screenshot right now but i will get one, will post in this thread. |
| // Stack-pinned, not `Box::pin`: this runs once per query, and boxing | ||
| // costs a malloc, a free and a memcpy of the whole exchange future | ||
| // every time. `Pin<&mut F>` is a `Future`, so `safe_timeout` takes it. | ||
| // | ||
| // The inner block matters: unlike the boxed future, which was moved | ||
| // into `safe_timeout` and dropped by the `.await`, `pin!` binds a local | ||
| // that would otherwise hold its borrow of `self` and `context` until | ||
| // the end of the function, past the arms below. |
There was a problem hiding this comment.
I'd prefer to see comments like this as part of the commit message rather than on the code itself. These often get out of sync with the code they're talking about and result in the code base being littered with inaccurate statements about why it's written a certain way. I'd only mention the borrow of self and context if the code fails to compile without the block
There was a problem hiding this comment.
sure i will take care of it
Right, but that's not inherently a problem. Replacing an allocation with a 10k stack variable is a tradeoff not an automatic win. The fact that you're seeing memcpy upon boxing implies something else is interfering with the optimizer. The compiler should be able to place the value in the box directly without having to copy it from the stack. |
will dig down more, need to know more on this issue |
|
To be clear, I don't think this change is 100% the wrong choice. I just don't think there's been sufficient investigation into what's going on. While this is a relatively hot path, one extra allocation per request should not be causing huge impacts on our performance. |
Thank you! |
|
starting from the root cause 
execute occupies almost 5.2% of the samples out of which have some mallocs and rjems 
|
|
Alright, I'm sufficiently convinced. If you can update this to pass CI I'll merge it. None of the |
ygxio
changed the title
self.client_server_exchange(context)self.client_server_exchange(context)
7e30898 to
0534116
Compare
|
This is perfect. Thanks for working on this! |
3 participants
closes #1395