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

Expand CreateSpan in T0 #91403

Merged
merged 4 commits into from
Oct 10, 2023
Merged

Expand CreateSpan in T0 #91403

merged 4 commits into from
Oct 10, 2023

Conversation

MichalPetryka
Copy link
Contributor

@MichalPetryka MichalPetryka commented Aug 31, 2023

I've noticed my app allocate 100MB of RuntimeFieldInfoStub instances in the VM implementation while running the T0 code in my app, expanding in T0 should avoid such allocations.

I've noticed my app allocate 100GB of `RuntimeFieldInfoStub` instances in the VM implementation while running the T0 code in my app, expanding in T0 should avoid such allocations.
@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, 2023
@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Aug 31, 2023
@ghost
Copy link

ghost commented Aug 31, 2023

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

Issue Details

I've noticed my app allocate 100GB of RuntimeFieldInfoStub instances in the VM implementation while running the T0 code in my app, expanding in T0 should avoid such allocations.

Author: MichalPetryka
Assignees: -
Labels:

area-CodeGen-coreclr

Milestone: -

@MichalPetryka
Copy link
Contributor Author

@jkotas Could this get a backport to 8.0?

@jkotas
Copy link
Member

jkotas commented Aug 31, 2023

It is up to the JIT team to make the call on a backport and bring it through the servicing process.

I do not think that this meets the bar for backport to .NET 8. It is by design that T0 compilation consumes extra memory, including extra GC allocations that would be eliminated by full JIT optimizations.

I've noticed my app allocate 100GB of RuntimeFieldInfoStub instances in the VM implementation

Why is not T1 compilation kicking fast enough for your app? It takes a while to allocate 100GB of RuntimeFieldInfoStub, I would expect T1 compilation to kick in a long before this much of RuntimeFieldInfoStub instances get allocated.

@MichalPetryka
Copy link
Contributor Author

It is by design that T0 compilation consumes extra memory, including extra GC allocations that would be eliminated by full JIT optimizations.

I understand that but with this optimalization pessimizing T0 code so much using it without AggressiveOptimization turns out to be a bad idea, making it effectively useless and a serious performance issue.

It takes a while to allocate 100GB of RuntimeFieldInfoStub

I've actually misread the data, it's able to allocate 100MB, not 100GB. The code in question consists of many hot loops which probably take a lot of time to OSR.

@EgorBo
Copy link
Member

EgorBo commented Aug 31, 2023

I agree with Jan that we already have tons of similar cases in tier0, e.g. Enum.HasFlag (already fixed), also everything related to devirtualizations of interfaces calls on structs when jit can see the exact type (see #90965) and other patterns where only optimized code can get rid of allocations.

We also know that sometimes a method may stay in Tier0 for too long because of the "promotion queue" problem (We had an idea to make it priority-based and e.g. all methods with allocators inside could get a higher priority).

@JulieLeeMSFT
Copy link
Member

@MichalPetryka, there are build errors. Please resolve it before merging.

@JulieLeeMSFT JulieLeeMSFT added this to the 9.0.0 milestone Sep 1, 2023
@MichalPetryka
Copy link
Contributor Author

@MihuBot -tier0

@EgorBo
Copy link
Member

EgorBo commented Sep 2, 2023

@MichalPetryka @MihaZupan is MihuBot failing because of this PR or it's a general issue? Does it fail on other changes?

@MihaZupan
Copy link
Member

It's failing somewhere in jit-diff. I'll try it on different PRs now.

@MihaZupan
Copy link
Member

Seems like the issue is specifically with this PR and tier 0.
It's working fine on other PRs, or on this PR without the --tier0 flag.

@EgorBo
Copy link
Member

EgorBo commented Sep 2, 2023

/azp list

@azure-pipelines

This comment was marked as resolved.

@EgorBo
Copy link
Member

EgorBo commented Sep 2, 2023

/azp run runtime-coreclr pgo, runtime-coreclr libraries-pgo

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@EgorBo
Copy link
Member

EgorBo commented Sep 2, 2023

Let's run some pipelines which don't disable TieredCompilation because green inner-loop CI likely means it was never executed in Tier0.

@EgorBo
Copy link
Member

EgorBo commented Sep 4, 2023

/azp run runtime-coreclr outerloop, runtime-coreclr gcstress0x3-gcstress0xc

@EgorBo
Copy link
Member

EgorBo commented Sep 4, 2023

/azp run runtime-coreclr outerloop, runtime-coreclr gcstress0x3-gcstress0xc

Oops, wrong PR 😄

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@EgorBo
Copy link
Member

EgorBo commented Oct 2, 2023

@MihuBot -tier0

@EgorBo
Copy link
Member

EgorBo commented Oct 10, 2023

I've ran jitdiff locally with --tier0 and this PR didn't seem to cause any issues for me so I am going to merge it.

Thanks, @MichalPetryka!

@EgorBo EgorBo merged commit c9b5dd4 into dotnet:main Oct 10, 2023
122 of 127 checks passed
@ghost ghost locked as resolved and limited conversation to collaborators Nov 10, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants