-
Notifications
You must be signed in to change notification settings - Fork 723
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
Improve speed of Randomizer.GetString #4512
Open
CollinAlpert
wants to merge
4
commits into
nunit:master
Choose a base branch
from
CollinAlpert:getrandomstring_speed_up
base: master
Could not load branches
Branch not found: {{ refName }}
Could not load tags
Nothing to show
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 1 commit
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use
int
isovar
fori
Also of you use the same names for the parameters as the other version makes this look similar.
I wonder if you can make a local function for the loop as that is exactly the same between both versions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I renamed the parameters and used
int
.As for making a local function, that would add an extra allocation for the closure and we wouldn't get as much of a performance benefit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if you make it local and mark it with the MethodImpAttribute(MethodImplOptions.AggressiveInlining) ? Does that affect performance?
And, you measure this in release, right?
Sorry for being a bit outdated: The
inline
keyword should be enough, and then the code can be beautiful and still performant :-)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That might help with the allocation (I'd need to check if the JIT would actually be able to inline the function), but that still wouldn't solve the problem of finding a common type between
Span<char>
andchar[]
. Any magic performed to put both of those types in a common function will definitely result in worse performance.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A
char[]
can be assigned to aSpan<char>
also on .NET Framework. So both could useSpan<char>
.I don't know if that changes any optimizations done by the JITter.
Could you put the below in your benchmark please, prevents me from having to write one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
However I have just run a benchmark with the updated code and it seems the method is not inlined and the performance is a bit worse than the NetFx code:
Please note that I do not have net472 installed and am running the net472 code on net8.0, so the
string.Create
on >= net6.0 version will still be faster than the stackalloc version on net472. It will just not be as performant as it could be.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure what you are comparing here? .NET Core vs NET Framework or old vs new?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have no way of providing a realistic benchmark, since I can't install net472. In these benchmarks I am comparing the PR code for net472 and >= net6.0, but running both benchmarks on net8.0. Unfortunately that's the best I can do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My benchmark results show that for .NET48 nor NET6.0 the new code is not improving things or in case of .NET48 even making things slower, but for NET8.0 the improvement is a lot.
What I haven't tested is to see if a .NET6.0 build of NUNIT when run under NET8.0 would give the same improvement as we are not intending to make an NET8.0 binary Nuget package.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does your benchmark do for
GetStringNew
in net48, since it can't use thestring.Create
API? Does it usestackalloc
?I managed to run some benchmarks on a Windows machine and also noticed that
stackalloc
was slower thanchar[]
on net48, so I will probably revert that change.