- Notifications
You must be signed in to change notification settings - Fork5.2k
Don't define HAS_CUSTOM_BLOCKS on mono#106764
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
EgorBo commentedAug 21, 2024
@EgorBot -mono --filter System.Memory.ReadOnlyMemory.ToArray |
EgorBot commentedAug 21, 2024
Benchmark results on Intel
|
EgorBo commentedAug 21, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@matouskozak PTAL, it fixes the issue according to the benchmark in#106764 (comment) |
matouskozak commentedAug 21, 2024
/azp run runtime-extra-platforms |
| Azure Pipelines successfully started running 1 pipeline(s). |
matouskozak left a comment
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?
BrzVlad commentedAug 22, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
We do unroll copies, in |
matouskozak commentedAug 22, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Looking at the codepaths for |
BrzVlad commentedAug 22, 2024
That should still emit a |
matouskozak commentedAug 22, 2024
We might want to backport this fix to .NET 9 due to the severity of the perf regressionsdotnet/perf-autofiling-issues#33182@jkurdek@vitek-karas.
Thank you, you're right. However, we are taking this path
mini_emit_memcpy because we passsize / align > MAX_INLINE_COPIES (klass min_align is 1). As a future work, we could try tweaking theMAX_INLINE_COPIES heuristics. |
EgorBo commentedAug 22, 2024
/backport to release/9.0 |
Started backporting to release/9.0:https://github.com/dotnet/runtime/actions/runs/10505141188 |
xtqqczze commentedAug 22, 2024
Is there an issue tracking this missed optimization? |
matouskozak commentedAug 22, 2024
I'm not aware of it so I created it#106822. |
This reverts commit7266021.
This reverts commit7266021.
Uh oh!
There was an error while loading.Please reload this page.
Fixesdotnet/perf-autofiling-issues#33182 regressions