Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add non-capturing overloads of Modify methods #1396

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from

Conversation

Joe4evr
Copy link
Contributor

@Joe4evr Joe4evr commented Oct 8, 2019

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.

@FiniteReality
Copy link
Member

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?

@Joe4evr
Copy link
Contributor Author

Joe4evr commented Oct 8, 2019

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

@Joe4evr Joe4evr force-pushed the NoCaptureOverloads branch from 301efd7 to 391823b Compare October 8, 2019 20:14
@Joe4evr
Copy link
Contributor Author

Joe4evr commented Oct 10, 2019

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
Copy link
Contributor Author

Joe4evr commented Oct 26, 2019

@foxbot PTAL?

@quinchs
Copy link
Member

quinchs commented Feb 3, 2022

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants