Skip to content

[release/6.0][mono][jit] Don't memset memory in increments lower than word size#74351

Merged
carlossanlop merged 1 commit intodotnet:release/6.0from
BrzVlad:backport-6.0-gc-memset
Sep 7, 2022
Merged

[release/6.0][mono][jit] Don't memset memory in increments lower than word size#74351
carlossanlop merged 1 commit intodotnet:release/6.0from
BrzVlad:backport-6.0-gc-memset

Conversation

@BrzVlad
Copy link
Member

@BrzVlad BrzVlad commented Aug 22, 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.

This was uncovered by crashes in CI in the System.Text.Json.Tests running on Mono(#58828)

Customer Impact
Infrequent crashes in the GC are possible on 64 bit targets where we use preemptive suspend or concurrent GC. (Desktop and mobile targets) without the fix.
Also speeds up memset by about 15% for smaller valuetypes hitting this path on 64bit. More for extra large valuetypes.

Testing
This was tested as part of our CI test suites on main for last 2 weeks without any detected failures. The changed code is hit very frequently, as part of clearing of medium sized / large valuetypes (initobj opcode).
Manual validation done prior to submission to main.

Risk
Low Risk. The change itself is very basic and it got tested over the last few weeks as part of our test suites.

Backport of #73480

…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.
@BrzVlad BrzVlad requested a review from marek-safar as a code owner August 22, 2022 15:51
@ghost ghost added the area-CoreLib-mono label Aug 22, 2022
@ghost ghost assigned BrzVlad Aug 22, 2022
@marek-safar marek-safar added this to the 6.0.x milestone Aug 22, 2022
@marek-safar marek-safar added the Servicing-consider Issue for next servicing release review label Aug 22, 2022
@SamMonoRT SamMonoRT added the Servicing-approved Approved for servicing release label Aug 24, 2022
@SamMonoRT
Copy link
Member

Approved via email. Marking as servicing approved

@SamMonoRT SamMonoRT removed the Servicing-consider Issue for next servicing release review label Aug 25, 2022
@carlossanlop
Copy link
Contributor

carlossanlop commented Sep 7, 2022

The single CI failure was infra related (and the leg got stuck too):

##[warning]The macOS-10.15 environment is deprecated, consider switching to macos-11(macos-latest), macos-12 instead. For more details see https://github.com/actions/virtual-environments/issues/5583
,##[error]An error occurred while provisioning resources (Error Type: Disconnect).
,##[warning]Received request to deprovision: The request was cancelled by the remote provider.

Approved and signed off. Can merge now.

@carlossanlop carlossanlop merged commit 26d6472 into dotnet:release/6.0 Sep 7, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Oct 8, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Servicing-approved Approved for servicing release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants