Discord.Net icon indicating copy to clipboard operation
Discord.Net copied to clipboard

Add non-capturing overloads of Modify methods

Open Joe4evr opened this issue 6 years ago • 5 comments

This is technically a breaking change since it modifies the interfaces, but people shouldn't be making their own implementations of our interfaces.

This change adds an overload to all ModifyAsync methods (that I could find) to take in a state parameter that's forwarded into the callback so they can be written without closing over local state. End users can take advantage of these overloads to keep GC pressure down.

Joe4evr avatar Oct 08 '19 19:10 Joe4evr

Pretty sure we ended up deciding that interface additions weren't breaking version changes, due to the internal-implementation-only part of them.

Overall, I'm not sure this is actually a worthwhile change given the fact that we're already a pretty allocation-heavy library. To me, it seems pretty hard to get any meaningful results, but could you run some benchmarks to check how much of an improvement this is?

FiniteReality avatar Oct 08 '19 19:10 FiniteReality

I'll write some tomorrow, it's getting late here.

Joe4evr avatar Oct 08 '19 19:10 Joe4evr

So because I had to deal with Discord's rate limits to get these numbers, I'm cutting out the timing columns from this table:

Method Ratio RatioSD Gen 0 Gen 1 Gen 2 Allocated
UsingClosuresText 1.00 0.00 - - - 15.09 KB
UsingClosuresEmbed 1.05 0.08 - - - 15.80 KB
UsingNonClosuresText 0.97 0.15 - - - 15.07 KB
UsingNonClosuresEmbed 0.90 0.13 - - - 10.72 KB

Joe4evr avatar Oct 10 '19 18:10 Joe4evr

@foxbot PTAL?

Joe4evr avatar Oct 26 '19 11:10 Joe4evr

@Joe4evr Are you willing to fix merge conflicts? I like this PR! 😃

quinchs avatar Feb 03 '22 18:02 quinchs