Skip to content

[mono][jit] Don't memset memory in increments lower than word size#73480

Merged
BrzVlad merged 1 commit intodotnet:mainfrom
BrzVlad:fix-gc-concurrent-initobj
Aug 8, 2022
Merged

[mono][jit] Don't memset memory in increments lower than word size#73480
BrzVlad merged 1 commit intodotnet:mainfrom
BrzVlad:fix-gc-concurrent-initobj

Conversation

@BrzVlad
Copy link
Member

@BrzVlad BrzVlad commented Aug 5, 2022

If we memset a vt containing references, then, in order for preemptive GC and concurrent GC to work, writing to reference slots must happen atomically, otherwise GC might encounter a garbage ref.

Also speeds up memset by about 15% for smaller valuetypes hitting this path on 64bit. More for extra large valuetypes.

Fixes #58828

If we memset a vt containing references, then, in order for preemptive GC and concurrent GC to work, writing to reference slots must happen atomically, otherwise GC might encounter a garbage ref.

Also speeds up memset by about 15% for smaller valuetypes hitting this path on 64bit. More for extra large valuetypes.
@BrzVlad BrzVlad requested a review from marek-safar as a code owner August 5, 2022 17:47
@ghost ghost added the area-CoreLib-mono label Aug 5, 2022
@ghost ghost assigned BrzVlad Aug 5, 2022
@ghost
Copy link

ghost commented Aug 5, 2022

Tagging subscribers to this area: @BrzVlad
See info in area-owners.md if you want to be subscribed.

Issue Details

If we memset a vt containing references, then, in order for preemptive GC and concurrent GC to work, writing to reference slots must happen atomically, otherwise GC might encounter a garbage ref.

Also speeds up memset by about 15% for smaller valuetypes hitting this path on 64bit. More for extra large valuetypes.

Fixes #58828

Author: BrzVlad
Assignees: BrzVlad
Labels:

area-CoreLib-mono, area-GC-mono

Milestone: -

@BrzVlad
Copy link
Member Author

BrzVlad commented Aug 5, 2022

This bug can be easily reproduced in a matter of seconds with the following test case:

using System;
using System.Threading;

public struct VTWithRefs {
	public object o0, o1, o2, o3, o4, o5, o6, o7, o8, o9, o10;
}

public class Program {
	public static Program p;

	public static void GCTrigger () {
		int index = 0;
		int n = 100000;
		Program[] major_objs = new Program [n];
		while (true) {
			major_objs [index] = new Program ();
			index++;
			if (index == n)
				index = 0;
		}
	}

	public static void Main (string[] args)
	{
		const int n = 1000;
		VTWithRefs[] array = new VTWithRefs [n];
		string obj = "obj";

		Thread thread = new Thread (GCTrigger);
		thread.Start ();

		while (true) {
			for (int i = 0; i < n; i++) {
				array [i] = default (VTWithRefs);
				array [i].o0 = obj;
				array [i].o1 = obj;
				array [i].o2 = obj;
				array [i].o3 = obj;
				array [i].o4 = obj;
				array [i].o5 = obj;
				array [i].o6 = obj;
				array [i].o7 = obj;
				array [i].o8 = obj;
				array [i].o9 = obj;
			}
		}
	}
}

@lambdageek
Copy link
Member

/azp run runtime-extra-platforms

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@BrzVlad BrzVlad merged commit ced3d92 into dotnet:main Aug 8, 2022
@akoeplinger
Copy link
Member

Should we backport this to 6.0?

@lambdageek
Copy link
Member

@akoeplinger I think we should

@akoeplinger
Copy link
Member

/backport to release/6.0

@github-actions
Copy link
Contributor

Started backporting to release/6.0: https://github.com/dotnet/runtime/actions/runs/2835143319

@github-actions
Copy link
Contributor

@akoeplinger backporting to release/6.0 failed, the patch most likely resulted in conflicts:

$ git am --3way --ignore-whitespace --keep-non-patch changes.patch

Applying: [mono][jit] Don't memset memory in increments lower than word size
Using index info to reconstruct a base tree...
M	src/mono/System.Private.CoreLib/src/System/String.Mono.cs
Falling back to patching base and 3-way merge...
Auto-merging src/mono/System.Private.CoreLib/src/System/String.Mono.cs
CONFLICT (content): Merge conflict in src/mono/System.Private.CoreLib/src/System/String.Mono.cs
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 [mono][jit] Don't memset memory in increments lower than word size
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Error: The process '/usr/bin/git' failed with exit code 128

Please backport manually!

@akoeplinger
Copy link
Member

@BrzVlad please backport manually

BrzVlad added a commit to BrzVlad/runtime that referenced this pull request Aug 22, 2022
…otnet#73480)

If we memset a vt containing references, then, in order for preemptive GC and concurrent GC to work, writing to reference slots must happen atomically, otherwise GC might encounter a garbage ref.

Also speeds up memset by about 15% for smaller valuetypes hitting this path on 64bit. More for extra large valuetypes.
carlossanlop pushed a commit that referenced this pull request Sep 7, 2022
…73480) (#74351)

If we memset a vt containing references, then, in order for preemptive GC and concurrent GC to work, writing to reference slots must happen atomically, otherwise GC might encounter a garbage ref.

Also speeds up memset by about 15% for smaller valuetypes hitting this path on 64bit. More for extra large valuetypes.
@ghost ghost locked as resolved and limited conversation to collaborators Sep 10, 2022
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.

System.Text.Json.Tests crashing on Mono

3 participants