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

Replace unaligned casts in SpanHelpers.Memmove #98812

Merged
merged 5 commits into from
Apr 15, 2024
Merged

Conversation

xtqqczze
Copy link
Contributor

@xtqqczze xtqqczze commented Feb 22, 2024

Fix #83709.

No diffs expected.

IL size is reduced by ~140 bytes.

@xtqqczze xtqqczze marked this pull request as ready for review February 22, 2024 15:52
@EgorBo
Copy link
Member

EgorBo commented Feb 22, 2024

@MihuBot

@MichalPetryka
Copy link
Contributor

@MihuBot -arm64

@EgorBo
Copy link
Member

EgorBo commented Feb 24, 2024

Do you mind if we merge this after #98623 to avoid resolving conflicts there because that PR is already too big?

@MihaZupan MihaZupan added area-System.Buffers and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Feb 25, 2024
@ghost
Copy link

ghost commented Feb 25, 2024

Tagging subscribers to this area: @dotnet/area-system-buffers
See info in area-owners.md if you want to be subscribed.

Issue Details

Fix #83709

IL size is reduced by ~140 bytes.

Author: xtqqczze
Assignees: -
Labels:

area-System.Buffers

Milestone: -

@xtqqczze xtqqczze marked this pull request as draft February 25, 2024 11:17
@teo-tsirpanis teo-tsirpanis added the community-contribution Indicates that the PR has been added by a community member label Feb 26, 2024
@xtqqczze xtqqczze changed the title Replace unaligned casts in Buffer.Memmove Replace unaligned casts in SpanHelpers.Memmove Feb 28, 2024
Fix dotnet#83709
IL size is reduced by ~140 bytes.
@xtqqczze xtqqczze marked this pull request as ready for review March 27, 2024 22:54
@xtqqczze
Copy link
Contributor Author

@EgorBo Build analysis is green, could you please review.

@xtqqczze
Copy link
Contributor Author

@MihuBot -arm64

Copy link
Member

@EgorBo EgorBo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, lgtm

@EgorBo
Copy link
Member

EgorBo commented Aug 21, 2024

This PR seems to introduce a lot of regressions for Mono dotnet/perf-autofiling-issues#33182

@EgorBot -commit 5aee4f4 vs previous -mono --filter System.Memory.ReadOnlyMemory.ToArray

@EgorBot
Copy link

EgorBot commented Aug 21, 2024

Benchmark results on Intel
BenchmarkDotNet v0.13.13-nightly.20240311.145, Ubuntu 22.04.4 LTS (Jammy Jellyfish)
Intel Xeon Platinum 8370C CPU 2.80GHz, 1 CPU, 16 logical and 8 physical cores
  Job-USZURW : .NET 9.0.0 (42.42.42.42424) using MonoVM, X64 X86Base
  Job-CMPAXD : .NET 9.0.0 (42.42.42.42424) using MonoVM, X64 X86Base
PowerPlanMode=00000000-0000-0000-0000-000000000000  IterationTime=250ms  MaxIterationCount=20
MinIterationCount=15  WarmupCount=1
Method Toolchain Size Mean Error Ratio Gen0 Allocated Alloc Ratio
ToArray Main 512 968.0 ns 3.64 ns 1.00 0.2512 1.03 KB 1.00
ToArray PR 512 426.5 ns 1.54 ns 0.44 0.2514 1.03 KB 1.00

BDN_Artifacts.zip

@EgorBo
Copy link
Member

EgorBo commented Aug 21, 2024

^ toolchain is misleading, but PR means "before the PR" and "Main" means "PR change"

@EgorBo
Copy link
Member

EgorBo commented Aug 21, 2024

This PR seems to introduce a lot of regressions for Mono dotnet/perf-autofiling-issues#33182

@EgorBot -commit 5aee4f4 vs previous -mono --filter System.Memory.ReadOnlyMemory.ToArray --envvars MONO_VERBOSE_METHOD:Memmove

@EgorBot
Copy link

EgorBot commented Aug 21, 2024

Benchmark results on Intel
BenchmarkDotNet v0.13.13-nightly.20240311.145, Ubuntu 22.04.4 LTS (Jammy Jellyfish)
Intel Xeon Platinum 8370C CPU 2.80GHz, 1 CPU, 16 logical and 8 physical cores
  Job-JCVYSL : .NET 9.0.0 (42.42.42.42424) using MonoVM, X64 X86Base
  Job-FEPIGK : .NET 9.0.0 (42.42.42.42424) using MonoVM, X64 X86Base
EnvironmentVariables=MONO_VERBOSE_METHOD=Memmove  PowerPlanMode=00000000-0000-0000-0000-000000000000  IterationTime=250ms
MaxIterationCount=20  MinIterationCount=15  WarmupCount=1
Method Toolchain Size Mean Error Ratio Gen0 Allocated Alloc Ratio
ToArray Main 512 959.0 ns 2.36 ns 1.00 0.2520 1.03 KB 1.00
ToArray PR 512 421.1 ns 1.34 ns 0.44 0.2522 1.03 KB 1.00

BDN_Artifacts.zip

@EgorBo
Copy link
Member

EgorBo commented Aug 21, 2024

Asm diff: https://www.diffchecker.com/70tCMUog/ (not sure which is PR and which is Main)

@github-actions github-actions bot locked and limited conversation to collaborators Sep 21, 2024
@xtqqczze xtqqczze deleted the Memmove branch September 30, 2024 22:57
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Buffers community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Buffer.Memmove has unaligned casts
7 participants