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.

Sign up for GitHub

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

Jump to bottom

Revert change in StringBuilder.Append(char) #74885

Merged
merged 1 commit into from
Sep 1, 2022
Merged

Revert change in StringBuilder.Append(char) #74885

merged 1 commit into from
Sep 1, 2022

Conversation

jkotas
Copy link
Member

@jkotas jkotas commented Aug 31, 2022

The change has a bad interaction with inlining heuristics.

Fixes #74158. Partial revert of #67448.

@jkotas
Revert change in StringBuilder.Append(char)
64ea0db 
The change has a bad interaction with inlining heuristics.

Fixes dotnet#74158. Partial revert of dotnet#67448.
@dotnet-issue-labeler dotnet-issue-labeler bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Aug 31, 2022
@ghost ghost assigned jkotas Aug 31, 2022
@ghost
Copy link

ghost commented Aug 31, 2022

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

Issue Details

The change has a bad interaction with inlining heuristics.

Fixes #74158. Partial revert of #67448.

Author: jkotas
Assignees: -
Labels:

area-CodeGen-coreclr
Milestone: -

@stephentoub
Copy link
Member

I assume this does in fact cause the method to be inlined again?

@jkotas
Copy link
Member Author

jkotas commented Aug 31, 2022

I assume this does in fact cause the method to be inlined again?

Yes, in specific situations depending on the callsite, like in the BigInteger.Parse where it showed up on the radar in our perf runs.

A separate question is why BigInteger parsing uses StringBuilder to copy the parsed number around. It sounds like an opportunity for improvement.

@jkotas jkotas mentioned this pull request Sep 1, 2022
Closed
@jkotas
Copy link
Member Author

jkotas commented Sep 1, 2022

Test failures are #74896 and #74488

@jkotas jkotas merged commit fccff6e into dotnet:main Sep 1, 2022
@jkotas
Copy link
Member Author

jkotas commented Sep 1, 2022

/backport to release/7.0

@jkotas jkotas deleted the fix-67448 branch September 1, 2022 00:27
@github-actions
Copy link
Contributor

github-actions bot commented Sep 1, 2022

Started backporting to release/7.0: https://github.com/dotnet/runtime/actions/runs/2967991509

@github-actions github-actions bot mentioned this pull request Sep 1, 2022
Merged
@AndyAyersMS
Copy link
Member

AndyAyersMS commented Sep 2, 2022 •
edited by DrewScoggins
Loading

Regressions:

Improvements

Possibly stale PGO? @DrewScoggins can you check when we have had recent updates?

@DrewScoggins
Copy link
Member

Last update was from August 20th. We have been having failures linked from the new installer that we are trying to take that I am investigating now.

@EgorBo EgorBo mentioned this pull request Sep 28, 2022
Closed
18 tasks
@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.
Reviewers

@JulieLeeMSFT JulieLeeMSFT JulieLeeMSFT approved these changes

@jakobbotsch jakobbotsch jakobbotsch approved these changes

@stephentoub stephentoub stephentoub approved these changes

@EgorBo EgorBo Awaiting requested review from EgorBo

Assignees

@jkotas jkotas

Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Performance regressions in System.Numerics.Tests.Perf_BigInteger.Parse
6 participants
@jkotas @stephentoub @AndyAyersMS @DrewScoggins @jakobbotsch @JulieLeeMSFT