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

Buffer::BlockCopy may spend too long without GC polling #13554

Closed
VSadov opened this issue Oct 9, 2019 · 9 comments · Fixed by dotnet/coreclr#27216
Closed

Buffer::BlockCopy may spend too long without GC polling #13554

VSadov opened this issue Oct 9, 2019 · 9 comments · Fixed by dotnet/coreclr#27216
Assignees

Comments

@VSadov
Copy link
Member

VSadov commented Oct 9, 2019

Buffer::BlockCopy is an FCALL that delegates to the native memmove which may spend a lot of time there depending on how much is being copied.

If GC needs to sync with user threads at inconvenient time, everything will stop until the memmove is done. GC would wait for memmove, everything else will wait for GC.
There was an actual scenario reported when GC pauses could take up to a minute due to this.
(dealing with very large streams, potentially swapped out, ...)

We should either "chunk" large copying into smaller pieces with intermittent GC polling, or just move the whole thing to managed code.

@VSadov VSadov closed this as completed Oct 9, 2019
@VSadov VSadov reopened this Oct 9, 2019
@jkotas
Copy link
Member

jkotas commented Oct 9, 2019

move the whole thing to managed code.

This. We have prior art in CoreRT. I will give it a shot

@jkotas jkotas self-assigned this Oct 9, 2019
@EgorBo
Copy link
Member

EgorBo commented Oct 9, 2019

Would be interesting to compare, in mono we emit @llvm.memmove intrinsic for Buffer.BlockCopy (and we ask llvm to place safepoints for us)

@jkotas
Copy link
Member

jkotas commented Oct 9, 2019

and we ask llvm to place safepoints for us

Do you have safepoints inside the loop that @llvm.memmove expands into when the length is not constant?

@EgorBo
Copy link
Member

EgorBo commented Oct 9, 2019

@jkotas just checked, unfortunately we don't, llvm is able to unroll it for small constants but otherwise it converts it into a libc call so the problem remains.

UPD: However, there is an ability to expand memmove into loops (expandMemMoveAsLoop) in LLVM IR it seems (and then -place-safepoints will be able to insert sp placeholders for us, will check)

@tannergooding
Copy link
Member

This. We have prior art in CoreRT. I will give it a shot

The memcpy routine that is distributed with MSVC (C:\Program Files (x86)\Microsoft Visual Studio\2019\Community\VC\Tools\MSVC\14.24.28202\crt\src\x64\memcpy.asm) currently defines a "large block" as 128-bytes (for non overlapping buffers) and uses prefetching and non-temporal stores for this scenario.

Is this something that CoreRT was handling?

@jkotas
Copy link
Member

jkotas commented Oct 9, 2019

Is this something that CoreRT was handling?

That is handled in both CoreCLR and CoreRT. Both delegate to CRT for blocks over certain size (with a proper PInvoke frame that avoids the GC starvation problem).

jkotas referenced this issue in jkotas/coreclr Oct 10, 2019
jkotas referenced this issue in jkotas/coreclr Oct 10, 2019
jkotas referenced this issue in jkotas/coreclr Oct 12, 2019
jkotas referenced this issue in dotnet/coreclr Oct 14, 2019
jkotas referenced this issue in jkotas/coreclr Oct 16, 2019
jkotas referenced this issue in jkotas/coreclr Oct 16, 2019
jkotas referenced this issue in jkotas/coreclr Oct 18, 2019
jkotas referenced this issue in dotnet/coreclr Oct 18, 2019
* Rewrite Buffer.BlockCopy in C#

Fixes #27106

* Workaround to enable type check optimizations for BlockCopy only
@jkotas
Copy link
Member

jkotas commented Oct 18, 2019

Keeping this open to fix other places where we do large copies in cooperative mode.

@jkotas jkotas reopened this Oct 18, 2019
jkotas referenced this issue in jkotas/coreclr Nov 2, 2019
jkotas referenced this issue in dotnet/coreclr Nov 3, 2019
@jkotas
Copy link
Member

jkotas commented Nov 5, 2019

This is fixed for all memory copy variants exposed by the framework now.

We have other similar problems for sure. I have opened dotnet/coreclr#27683 on one found via codereview. Also, @adamsitnik is going to run experiment to see whether Benchmark.NET can be used to find these types of issues.

@jkotas jkotas closed this as completed Nov 5, 2019
@adamsitnik
Copy link
Member

Also, @adamsitnik is going to run experiment to see whether Benchmark.NET can be used to find these types of issues.

I've performed the experiment and shared my results in dotnet/performance#1049

@msftgits msftgits transferred this issue from dotnet/coreclr Jan 31, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Dec 12, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants