[release/7.0-rc2] [Mono] Restore old code to solve the recent SpanHelpers regressions #75996

github-actions · 2022-09-22T00:56:48Z

Backport of #75917 to release/7.0-rc2. This replaces backport PR #75932.

Customer Impact

Fixes large performance regression #75709 in library code when running on mono (AOT, interpreter) introduced in rc1 (#74086)

Testing

Benchmarks were manually run before merge into main to ensure we were seeing expected recovery. We also have the automated benchmarks running now, and we will update with the results once available.

Risk

We explored several approaches to this fix along the way. We concluded that restoring the old implementation of these methods for use on mono only was the best bet, but those had to be adapted to align with other refactoring that had been done. The result is introducing a SpanHelpers.Mono.cs file with implementations that don't use INumber<T> and carry the previous intrinsics-based implementations.

This approach keeps the CoreCLR performance improvements intact while avoiding the mono regression.

ghost · 2022-09-22T00:57:01Z

Tagging subscribers to this area: @dotnet/area-system-memory
See info in area-owners.md if you want to be subscribed.

Issue Details

Backport of #75917 to release/7.0-rc2

/cc @jeffhandley @adamsitnik

Customer Impact

Testing

Risk

IMPORTANT: Is this backport for a servicing release? If so and this change touches code that ships in a NuGet package, please make certain that you have added any necessary package authoring and gotten it explicitly reviewed.

Author:	github-actions[bot]
Assignees:	-
Labels:	`area-System.Memory`
Milestone:	-

stephentoub · 2022-09-22T01:13:21Z

src/libraries/System.Private.CoreLib/src/System/SpanHelpers.T.cs

 [MethodImpl(MethodImplOptions.AggressiveInlining)]
 internal static int LastIndexOfAnyValueType<T>(ref T searchSpace, T value0, T value1, T value2, T value3, int length) where T : struct, INumber<T>
 => LastIndexOfAnyValueType<T, DontNegate<T>>(ref searchSpace, value0, value1, value2, value3, length);

 [MethodImpl(MethodImplOptions.AggressiveInlining)]
 internal static int LastIndexOfAnyExceptValueType<T>(ref T searchSpace, T value0, T value1, T value2, T value3, int length) where T : struct, INumber<T>
 => LastIndexOfAnyValueType<T, Negate<T>>(ref searchSpace, value0, value1, value2, value3, length);
+#endif

 [MethodImpl(MethodImplOptions.AggressiveOptimization)]
 private static int LastIndexOfAnyValueType<TValue, TNegator>(ref TValue searchSpace, TValue value0, TValue value1, TValue value2, TValue value3, int length)


Did we validate that code like this which should be unreferenced in the mono build is correctly being trimmed away?

I just pulled down and built the change, and it looks like almost everything is. The INegator<T> interface isn't being removed, but that's small potatoes.

Thanks, @stephentoub. I'd be happy to follow this up with another PR for GA to ifdef that orphaned code away, but I wasn't able to explore that today.

The INegator interface isn't being removed.

What is that? IL linker bug?

@stephentoub / @jkotas - Should we expect action from this comment in this PR?

Should we expect action from this comment in this PR?

No

What is that? IL linker bug?

I believe so, but I've not investigated further yet.

carlossanlop · 2022-09-22T14:33:26Z

This was approved by Tactics via chat.

carlossanlop · 2022-09-22T15:33:27Z

Adam wrote this on the previous PR: #75932 (comment)

How are we looking here?

The results I got don't prove that all the regressions are gone: #75917 (comment)

@radekdoulik has shared offline results that show that the fix actually helps. He has run the benchmarks in different way that I did, using perf pipeline developed by @radical.

I leave the decision up to @radekdoulik and @radical who own WASM and have more experience in benchmarking it.

@radekdoulik @radical can you confirm the perf looks good, and if it does, can you please sign off?

lewing

Lets take this now

* Fix querying L3 cache size on osx-x64 (#75870) Co-authored-by: Filip Navara <navara@emclient.com> * KeyChar should be preserved for Ctrl+Letter (#75861) Co-authored-by: Adam Sitnik <adam.sitnik@gmail.com> * Try re-enabling System.Transactions.Local tests on Arm64 (#75703) Co-authored-by: Jan Kotas <jkotas@microsoft.com> * Updating inbox source generators to Roslyn 4.4 and removing polyfill approach (#75717) (#75875) * Removed internalProperties group from proxy and tests. (#75906) Co-authored-by: Ilona Tomkowicz <itomkowicz@microsoft.com> * [release/7.0-rc2] Fixing SpanHelpers.LastIndexOfAnyValueType to no longer create out of bounds GC refs (#75885) * Fixing SpanHelpers.LastIndexOfAnyValueType to no longer create out of bounds GC refs * Apply suggestions from code review Co-authored-by: Adam Sitnik <adam.sitnik@gmail.com> * Adjusting the comment as per PR review feedback Co-authored-by: Tanner Gooding <tagoo@outlook.com> Co-authored-by: Adam Sitnik <adam.sitnik@gmail.com> * Bump intellisense package for RC2 to include latest comments for Numerics (#75938) Co-authored-by: carlossanlop <carlossanlop@users.noreply.github.com> * [release/7.0-rc2] [Mono] Restore old code to solve the recent SpanHelpers regressions (#75996) * bring back the old code... * bring back more old code * Use an ifdef around clr code instead of a separate file * Delete SpanHelpers.Clr.cs * Remove a remaining INumber<T> helper from mono Co-authored-by: Adam Sitnik <adam.sitnik@gmail.com> Co-authored-by: Jeff Handley <jeffhandley@users.noreply.github.com> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Filip Navara <navara@emclient.com> Co-authored-by: Adam Sitnik <adam.sitnik@gmail.com> Co-authored-by: Jan Kotas <jkotas@microsoft.com> Co-authored-by: Jose Perez Rodriguez <joperezr@microsoft.com> Co-authored-by: Ilona Tomkowicz <itomkowicz@microsoft.com> Co-authored-by: Tanner Gooding <tagoo@outlook.com> Co-authored-by: Carlos Sanchez <1175054+carlossanlop@users.noreply.github.com> Co-authored-by: carlossanlop <carlossanlop@users.noreply.github.com> Co-authored-by: Jeff Handley <jeffhandley@users.noreply.github.com>

uweigand · 2022-10-07T17:42:06Z

I've seen frequent crashes with rc1 on s390x (Mono-based big-endian platform) - these were all segmentation faults in SpanHelpers.IndexOfValueType called from various different places. I haven't investigated in detail, but it looks like the code would access a few bytes beyond the end of the span, and if you're unlucky and that crosses a page boundary into a page that isn't mapped, you get a crash.

Applying the patch from this PR seems to fix those crashes for me, so it's apparently not just a performance fix.

adamsitnik · 2022-10-07T19:40:46Z

@uweigand what you are describing sounds like #75857

uweigand · 2022-10-12T07:57:47Z

@uweigand what you are describing sounds like #75857

Thanks for the pointer, but this turned out to be unrelated. (#75857 also only affects the vector code, and we don't currently support vectors in Mono on s390x, so this code would never trigger either way.)

The actual root cause was a codegen bug in Mono, where the OP_CHECK_THIS opcode could generate too-large memory accesses: #76915 . With this fixed, I'm no longer seeing any crashes on rc1.

adamsitnik and others added 5 commits September 22, 2022 00:56

bring back the old code...

851b6d7

bring back more old code

9f3d5a5

Use an ifdef around clr code instead of a separate file

e086ac4

Delete SpanHelpers.Clr.cs

7506a77

Remove a remaining INumber<T> helper from mono

a841473

dotnet-issue-labeler bot added the area-System.Memory label Sep 22, 2022

jeffhandley added tenet-performance Performance related issue Servicing-consider Issue for next servicing release review labels Sep 22, 2022

jeffhandley requested review from radical, lewing and jkotas September 22, 2022 01:07

stephentoub reviewed Sep 22, 2022

View reviewed changes

carlossanlop added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Sep 22, 2022

carlossanlop added this to the 7.0.0 milestone Sep 22, 2022

carlossanlop requested a review from radekdoulik September 22, 2022 15:34

lewing approved these changes Sep 22, 2022

View reviewed changes

jeffhandley approved these changes Sep 22, 2022

View reviewed changes

jeffhandley merged commit 550605c into release/7.0-rc2 Sep 22, 2022

lewing deleted the backport/pr-75917-to-release/7.0-rc2 branch September 22, 2022 19:34

lewing mentioned this pull request Sep 26, 2022

[Perf] Linux/x64: 27 Regressions on 8/17/2022 6:09:21 PM #74395

Closed

uweigand mentioned this pull request Oct 12, 2022

[mono][s390x] Wrong implementation of OP_CHECK_THIS #76915

Closed

teo-tsirpanis mentioned this pull request Nov 8, 2022

[mono] Array.IndexOf / Span<T>.IndexOf very slow on .net 7 #78008

Closed

ghost locked as resolved and limited conversation to collaborators Nov 11, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[release/7.0-rc2] [Mono] Restore old code to solve the recent SpanHelpers regressions #75996

[release/7.0-rc2] [Mono] Restore old code to solve the recent SpanHelpers regressions #75996

github-actions bot commented Sep 22, 2022 •

edited by jeffhandley

Loading

ghost commented Sep 22, 2022

Customer Impact

Testing

Risk

stephentoub Sep 22, 2022

stephentoub Sep 22, 2022

jeffhandley Sep 22, 2022

jkotas Sep 22, 2022

carlossanlop Sep 22, 2022

stephentoub Sep 22, 2022

carlossanlop commented Sep 22, 2022

carlossanlop commented Sep 22, 2022

lewing left a comment

uweigand commented Oct 7, 2022

adamsitnik commented Oct 7, 2022

uweigand commented Oct 12, 2022

[release/7.0-rc2] [Mono] Restore old code to solve the recent SpanHelpers regressions #75996

[release/7.0-rc2] [Mono] Restore old code to solve the recent SpanHelpers regressions #75996

Conversation

github-actions bot commented Sep 22, 2022 • edited by jeffhandley Loading

Customer Impact

Testing

Risk

ghost commented Sep 22, 2022

Customer Impact

Testing

Risk

stephentoub Sep 22, 2022

Choose a reason for hiding this comment

stephentoub Sep 22, 2022

Choose a reason for hiding this comment

jeffhandley Sep 22, 2022

Choose a reason for hiding this comment

jkotas Sep 22, 2022

Choose a reason for hiding this comment

carlossanlop Sep 22, 2022

Choose a reason for hiding this comment

stephentoub Sep 22, 2022

Choose a reason for hiding this comment

carlossanlop commented Sep 22, 2022

carlossanlop commented Sep 22, 2022

lewing left a comment

Choose a reason for hiding this comment

uweigand commented Oct 7, 2022

adamsitnik commented Oct 7, 2022

uweigand commented Oct 12, 2022

github-actions bot commented Sep 22, 2022 •

edited by jeffhandley

Loading