Support new impl traits upstream #696
Conversation
|
👋 Hi! I see this is a draft PR. |
tnull
left a comment
tnull
left a comment
There was a problem hiding this comment.
I have to say I'm not the biggest fan of all this additional boilerplate, especially since that will get even bigger when we finally support and make use of async versions of the MigratableKVStore and PaginatedKVStore. I also don't quite buy that the added complexity is worth saving some boxing, especially given that tokio will auto-box all read/write tasks as they are simply too big for the stack (according to tokio at least).
Note this already needs a rebase, and also doesn't compile under cargo build --features uniffi currently.
Given that this will likely introduce a lot of conflicts with #692 and make the changes there even more complex, I might be good to hold off on this (and by extension the upstream API-breaking changes until after #692 lands).
src/io/vss_store.rs
Outdated
| let secondary_namespace = secondary_namespace.to_string(); | ||
| let key = key.to_string(); | ||
| let inner = Arc::clone(&self.inner); | ||
| Box::pin(async move { |
There was a problem hiding this comment.
Wasn't the main idea to avoid these Boxes?
There was a problem hiding this comment.
Oops, yea, just missed these.
I think we'll survive three or four more methods :)
No, tokio will auto-box the top-of-task future, but everything below that gets compiled into a single object when you have concrete future types. This should nontrivially reduce boxing, which I imagine are a decent chunk of our allocations, or at least on ldk-sample write-related allocations were so I have to imagine still are. Of course we don't reduce allocations in
Seems reasonable to hold off on landing this, but I don't see why that has to delay landing the PR upstream? Its somewhat annoying to rebase a PR that touches a bunch of code across |
3 participants
Updates to support lightningdevkit/rust-lightning#4175 as described at lightningdevkit/rust-lightning#4175 (comment)
Commit messages need cleaning up and rustfmt, but otherwise its probably good.