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

[Revived PR] Faster Buffer.MemoryCopy for AMD64 #9143

Closed
wants to merge 3 commits into from

Conversation

jamesqo
Copy link

@jamesqo jamesqo commented Jan 26, 2017

This is a revival of the PR at #6638 I had to close a few months earlier because it had too many comments. I basically re-wrote the code I had written earlier, except with a few changes:

  • I removed the optimizations for ARM64. I know very little about that platform, so someone more qualified should probably optimize it.
  • I kept things mostly the way they were for non-AMD64 platforms. The only major thing I changed was replacing branching at the beginning/end of the copy with unaligned writes.
  • I very conservatively kept the P/Invoke threshold at 1024 bytes, because even though my benchmarks said that the managed implementation was faster up to 8192 bytes, @nietras' benchmarks were saying 1024.
    • After this is merged, I'll open up a new PR where I increase it back to 8192 and we can discuss more about the threshold size.
  • I also renamed some things to make the code cleaner, e.g. so that variables have self-explanatory names.

Please note: I am looking to get this merged mostly as-is because it's a clear win over what we currently have. Once this gets in, then we can open the door for further discussion/experimentation/etc. over optimizations.

/cc @jkotas, @nietras @tannergooding @benaadams FYI

@jamesqo
Copy link
Author

jamesqo commented Jan 26, 2017

Again this is largely a cleaned-up version of the same code I had written earlier, but I'll post benchmarks regardless. I have to re-build CoreCLR and write the benchmarks however, so I won't be able to do so straightaway. I'll update this PR when I have them.

@benaadams
Copy link
Member

benaadams commented Jan 26, 2017

What's the impact with #7198 ? Does it replace the pinvoke as suggested? (Would be in Win x64 only though)

@jamesqo jamesqo mentioned this pull request Feb 17, 2017
@jamesqo
Copy link
Author

jamesqo commented Feb 17, 2017

Going to get back to this sometime later-- have been busy lately

@jkotas
Copy link
Member

jkotas commented Feb 25, 2017

Superseded by #9786

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants