-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Don't define HAS_CUSTOM_BLOCKS on mono #106764
Conversation
@EgorBot -mono --filter System.Memory.ReadOnlyMemory.ToArray |
Benchmark results on Intel
|
@matouskozak PTAL, it fixes the issue according to the benchmark in #106764 (comment) |
/azp run runtime-extra-platforms |
Azure Pipelines successfully started running 1 pipeline(s). |
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.
Thank you for the fix and help with investigating the issue.
I don't think that MonoJIT (or other Mono codegens) have this guarantee to unroll blocks (<= 64 bytes) so I think this is a good fix. @BrzVlad do you know otherwise?
We do unroll copies, in |
Looking at the codepaths for |
That should still emit a |
We might want to backport this fix to .NET 9 due to the severity of the perf regressions dotnet/perf-autofiling-issues#33182 @jkurdek @vitek-karas.
Thank you, you're right. However, we are taking this path runtime/src/mono/mono/mini/memory-access.c Line 221 in 169e22c
mini_emit_memcpy because we pass size / align > MAX_INLINE_COPIES (klass min_align is 1). As a future work, we could try tweaking the MAX_INLINE_COPIES heuristics.
|
/backport to release/9.0 |
Started backporting to release/9.0: https://github.com/dotnet/runtime/actions/runs/10505141188 |
Is there an issue tracking this missed optimization? |
I'm not aware of it so I created it #106822. |
This reverts commit 7266021.
This reverts commit 7266021.
dotnet#107558) This reverts commit 7266021.
Fixes dotnet/perf-autofiling-issues#33182 regressions