Skip to content

Comments

Revert change in StringBuilder.Append(char)#74885

Merged
jkotas merged 1 commit intodotnet:mainfrom
jkotas:fix-67448
Sep 1, 2022
Merged

Revert change in StringBuilder.Append(char)#74885
jkotas merged 1 commit intodotnet:mainfrom
jkotas:fix-67448

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.
@ghost ghost 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

stephentoub commented Aug 31, 2022

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

@JulieLeeMSFT JulieLeeMSFT requested a review from EgorBo August 31, 2022 18:30
@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

DrewScoggins commented Sep 7, 2022

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

@stephentoub stephentoub stephentoub approved these changes

@jakobbotsch jakobbotsch jakobbotsch approved these changes

@JulieLeeMSFT JulieLeeMSFT JulieLeeMSFT 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