-
-
Notifications
You must be signed in to change notification settings - Fork 120
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
[Shared] Optimized sequence equal #257
[Shared] Optimized sequence equal #257
Conversation
(cherry picked from commit c3b33dcbafb201706c9bf7ff201cc38e05b2bfca)
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.
Can add comments explaining what the code does or name the variables more clearly? It's hard to understand some of it.
How about this. I had ChatGPT write most of it, except for the Note at the end. No really, it's amazing. I gave him the code and asked him to add a comment and he did. |
Change variable names as well. |
fixed |
That's better, thanks. Someone suggested that this might be even faster or at least simpler. |
Is the method of using SIMD? I suppose if I were to write and use native functions in C it would be possible, but ... |
I meant specifically the code in lines 931 to 950. It seems to not use anything special outside the Unsafe class and looks similar to your solution. |
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.
Looks good. It might make sense to move SequenceEqual into a utility class and use it in other places in the code. I believe it's used a bunch inside IllusionFixes too. Maybe rename it to SequenceEqualFast?
Oh, I see, that way. |
Time for a 4MB comparison:
536.2[us]
425.5[us]
409[us]
314.9[us]
278.4[us]
|
A new function called SequenceEqualFast was used. 349.6[us] NG
|
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.
Looks good, good work!
The method could be added to KKAPI so that other plugins can use it easily instead of copying it. OverlayMod would probably benefit from it.
I experimented to see if I could adapt the following code in Core.UncensorSelector.Controller.cs.
System.Linq.SequenceEqual: 12[ms] Ver1:
ver2:
ver3:
|
I think the ver3 is still worthwhile to add, especially since it's so straight forward. In theory there may be some uncensor with a very large mesh, which would have a bigger effect on time. I think |
Ah, the high poly model.
Why? |
You're right that it doesn't matter in C#, but to people used to C and C++ seeing |
I'm good with C++, but I know of cases where i++ can cause performance problems. So i++ looks more unnatural. It's not a penalty that happens with int types, and most modern compilers can avoid it. |
Should ver3 also be implemented in KKAPI and SequenceEqualFast in KKPlugin be removed? |
It would be a good idea to include it in KKAPI, yes. Other common variations too maybe. |
Optimized SequenceEqual. Please merge if you have no problem.
The loading time for scenes containing large images with the same content is slightly reduced.
This change should also slightly reduce the loading time of apng/gifs.
Loading time for one huge scene
Before: average 33.78[s]
After: average 32.04[s] (-1.74)
Linq's SequenceEqual was taking 40ms to compare 4MB of data.
https://github.com/mono/mono/blob/c6cdaadb54a1173484f1ada524306ddbf8c2e7d5/mcs/class/referencesource/System.Core/System/Linq/Enumerable.cs#L944
I think the implementation is this SequenceEqual.
I think the overhead of MoveNext() and comparer in the Enumerator is the cause of the slowdown.
The large GCAlloc may also cause boxing by reference to Current.
I hadn't looked at the profiler in a while, and there was something at the top. So I looked into it.
Before Profile:
MonoProfilerOutput_2024-06-01_13-41-13.csv
After Profile:
MonoProfilerOutput_2024-06-01_13-48-02.csv