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/6.0] Remove usage of [AggressiveOptimization] #58253

Merged
merged 1 commit into from
Sep 1, 2021

Conversation

github-actions[bot]
Copy link
Contributor

@github-actions github-actions bot commented Aug 27, 2021

Backport of #58209 to release/6.0

/cc @stephentoub @steveharter

Customer Impact

Removing the usages of [AggressiveOptimization] is suggested per the JIT team. It has the potential to increase performance especially when R2R code is not used and PGO (Profile Guided Optimization) kicks in after many runs.

Testing

Local benchmarks show that when enabling PGO features (DOTNET_ReadyToRun=0, DOTNET_TC_QuickJitForLoops=1, DOTNET_TieredPGO=1) show up to 10% improvement in a serializer overhead benchmark.

Risk

The expected perf gains are expected to outweigh any possible minor perf regressions; performance will be determined by the jitter and environment settings and not influenced by [AggressiveOptimization].

@ghost
Copy link

ghost commented Aug 27, 2021

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

Issue Details

Backport of #58209 to release/6.0

/cc @stephentoub @steveharter

Customer Impact

Testing

Risk

Author: github-actions[bot]
Assignees: -
Labels:

area-System.Text.Json

Milestone: -

@danmoseley
Copy link
Member

@steveharter could you fill out a brief template for me

@danmoseley
Copy link
Member

This one is borderline for me as it seems there's no regression we're fixing in the mainline case. But presumably the gap between R2R and not R2R has increased over 5.0? Also I see that the codegen team guidance is that this is an antipattern we should avoid. Arguably this also reduces risk by returning us to 5.0 behavior.

is that right?

@steveharter
Copy link
Member

Also I see that the codegen team guidance is that this is an antipattern we should avoid. Arguably this also reduces risk by returning us to 5.0 behavior

Yes that is correct. Also removing the attributes this supports the new PGO functionality, but my understanding is that is has to be manually turned on and currently is still somewhat "experimental".

@Anipik
Copy link
Contributor

Anipik commented Aug 31, 2021

cc @danmoseley

@danmoseley
Copy link
Member

OK, approved per reasoning above.

@stephentoub stephentoub merged commit d228aa9 into release/6.0 Sep 1, 2021
@stephentoub stephentoub deleted the backport/pr-58209-to-release/6.0 branch September 1, 2021 13:37
@ghost ghost locked as resolved and limited conversation to collaborators Oct 1, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants