Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

Fixing Buffer::BlockCopy, JIT_MemCpy, and JIT_MemSet to just call the appropriate CRT functions for x64 Windows, as is already done for all other platforms/targets #25763

Merged
merged 3 commits into from
Jul 20, 2019

Conversation

tannergooding
Copy link
Member

This resolves #25505 for release/3.0 and is a backport of #25750.

Based on the original issue where JIT_MemCpy was changed to use rep movsb (see #7198), there was:

minor improvement (~5%) for arrays of length 0 to 120
good improvement (~40%) for arrays of length 130 to 510
~47% for arrays of length 130 to 310
~39% for arrays of length 320 to 440
~27% for arrays of length 450 to 510
little improvement (~1%) for arrays above 510 in length
This was only tested for 520 and 1000 bytes
However, on AMD processors, there are additional limitations around rep movsb and when it is beneficial to use. The common conditions under which it is being used in the JIT_MemCpy method today actually cause a 2x perf decrease for arrays larger than 512 bytes.

Having a custom memcpy routine adds additional maintenance burden, can be error prone, is generally not as widely tested, and does not get many of the optimizations that the CRT implementations receives. This, coupled with the overall minor improvements for small arrays on Intel processors and the 2x regression for arrays over 512 bytes on AMD processors is resulting in the custom memcpy routine being removed.

It would be beneficial for any future improvements to memcpy to be made directly against glibc and crt instead.

@tannergooding
Copy link
Member Author

CC. @MeiChin-Tsai, @jkotas, @janvorli

@tannergooding
Copy link
Member Author

Closed and reopened as per https://github.com/dotnet/corefx/issues/39593

@danmoseley
Copy link
Member

danmoseley commented Jul 18, 2019

@MeiChin-Tsai this one is your call 😄 I would have a preference to take it, if the risk is low enough because it's a large (2x) regression on a significant proportion of CPU out there, in fairly commonly used API, that we missed due to insufficient coverage of this hardware. However, I'd maybe not take it in ask mode.

@jkotas how do you feel about the risk? @tannergooding is this close to a git revert (which would be lower risk)?

@tannergooding
Copy link
Member Author

@tannergooding is this close to a git revert (which would be lower risk)?

No, it isn't a revert. A revert would have kept us with a custom JIT_MemCpy and JIT_MemSet implementation for x64 Windows. Instead, this just makes x64 Windows consistent with the other platform/architecture combinations and just forwards to the CRT implementation.

@danmoseley
Copy link
Member

This was approved offline.

@tannergooding
Copy link
Member Author

/AzurePipelines help

@azure-pipelines
Copy link

Supported commands
  • help:
    • Get descriptions, examples and documentation about supported commands
    • Example: help "command_name"
  • list:
    • List all pipelines for this repository using a comment.
    • Example: "list"
  • run:
    • Run all pipelines or a specific pipeline for this repository using a comment. Use this command by itself to trigger all related pipelines, or specify a pipeline to run.
    • Example: "run" or "run pipeline_name"
  • where:
    • Report back the Azure DevOps orgs that are related to this repository and org
    • Example: "where"

See additional documentation.

@tannergooding
Copy link
Member Author

/AzurePipelines list

@azure-pipelines
Copy link

CI/CD Pipelines for this repository:

@tannergooding
Copy link
Member Author

/azp run coreclr-ci

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@tannergooding
Copy link
Member Author

Rebase to try and resolve AZP failures. Several jobs weren't reporting statuses back.

@danmoseley danmoseley requested a review from jkotas July 19, 2019 23:25
@tannergooding tannergooding merged commit cb650e0 into dotnet:release/3.0 Jul 20, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants