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

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

Closed

Conversation

github-actions[bot]
Copy link
Contributor

@github-actions github-actions bot commented Sep 20, 2022

Backport of #75917 to release/7.0-rc2

/cc @radical @adamsitnik

Customer Impact

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

Testing

Automated benchmark is running (will add results)

Risk

Medium/Low - The patch restores the previous code path for mono

@ghost
Copy link

ghost commented Sep 20, 2022

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 @radical @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: -

@carlossanlop
Copy link
Member

@adamsitnik @radical please fill out the template, add the servicing-consider label, and email Tactics today, so in case it gets approved, I merge it tonight.

cc @mmitche, we have one more.

@lewing lewing added the Servicing-consider Issue for next servicing release review label Sep 20, 2022
@lewing
Copy link
Member

lewing commented Sep 20, 2022

@adamsitnik @radical please fill out the template, add the servicing-consider label, and email Tactics today, so in case it gets approved, I merge it tonight.

cc @mmitche, we have one more.

we're going to let the perf pipeline run finish first but Steve is aware of this one

@radical
Copy link
Member

radical commented Sep 21, 2022

The helix jobs for wasm/perf finally started running after being queued up for 2hrs. Build - 20220920.13 on dotnet-runtime-perf. And this will now take another ~1.5hours to complete :/

@radical
Copy link
Member

radical commented Sep 21, 2022

In the earlier runs 62db96b was missing because it was merged after the last changes here.

New runs:

@mmitche
Copy link
Member

mmitche commented Sep 21, 2022

@carlossanlop How close on this are we? Can we get this in ASAP?

@adamsitnik
Copy link
Member

How close on this are we? Can we get this in ASAP?

I am waiting for local benchmarks run to compare the results and verify that the PR solves the regressions. According to the tool it needs two more hours.

@carlossanlop
Copy link
Member

The single failure is networking, unrelated (infra, timeout): #58628

@mmitche
Copy link
Member

mmitche commented Sep 21, 2022

How close on this are we? Can we get this in ASAP?

I am waiting for local benchmarks run to compare the results and verify that the PR solves the regressions. According to the tool it needs two more hours.

@adamsitnik How are we looking here?

@adamsitnik
Copy link
Member

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.

jeffhandley and others added 2 commits September 21, 2022 17:39
…75917)

* 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: Jeff Handley <jeffhandley@users.noreply.github.com>
@jeffhandley
Copy link
Member

I was worried I'd get the backport merge update wrong and I did. I'm going to close this PR and do a fresh backport from #75917 now that it's merged.

@jeffhandley jeffhandley deleted the backport/pr-75917-to-release/7.0-rc2 branch September 22, 2022 00:53
@ghost ghost locked as resolved and limited conversation to collaborators Oct 22, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Memory Servicing-consider Issue for next servicing release review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants