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

Mark final types as exact #88163

Merged
merged 5 commits into from
Jul 18, 2023
Merged

Mark final types as exact #88163

merged 5 commits into from
Jul 18, 2023

Conversation

MichalPetryka
Copy link
Contributor

Should let the JIT devirtualize way more stuff due to knowing the exact types.

Extracted from #87579.

@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 Jun 28, 2023
@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Jun 28, 2023
@ghost
Copy link

ghost commented Jun 28, 2023

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

Issue Details

Should let the JIT devirtualize way more stuff due to knowing the exact types.

Extracted from #87579.

Author: MichalPetryka
Assignees: -
Labels:

area-CodeGen-coreclr

Milestone: -

@EgorBo
Copy link
Member

EgorBo commented Jun 29, 2023

/azp list

@azure-pipelines
Copy link

CI/CD Pipelines for this repository:

@EgorBo
Copy link
Member

EgorBo commented Jun 29, 2023

/azp run runtime-coreclr outerloop, runtime-coreclr jitstress, runtime-extra-platforms, runtime-coreclr pgostress

@azure-pipelines
Copy link

Azure Pipelines successfully started running 4 pipeline(s).

@EgorBo
Copy link
Member

EgorBo commented Jun 29, 2023

/azp run runtime-coreclr outerloop, runtime-coreclr jitstress, runtime-extra-platforms, runtime-coreclr pgostress

@azure-pipelines
Copy link

Azure Pipelines successfully started running 4 pipeline(s).

@MichalPetryka
Copy link
Contributor Author

@MihuBot

@EgorBo
Copy link
Member

EgorBo commented Jun 30, 2023

/azp run runtime-coreclr outerloop, runtime-coreclr jitstress, runtime-extra-platforms, runtime-coreclr pgostress

@azure-pipelines
Copy link

Azure Pipelines successfully started running 4 pipeline(s).

Copy link
Member

@AndyAyersMS AndyAyersMS left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like we ought to put the calls to getExactClasses into impIsClassExact to centralize the logic, and then just have these methods simply call impIsClassExact if they don't have exact classes.

@EgorBo
Copy link
Member

EgorBo commented Jul 7, 2023

Seems like we ought to put the calls to getExactClasses into impIsClassExact to centralize the logic, and then just have these methods simply call impIsClassExact if they don't have exact classes.

As far as I know that's not needed because impIsClassExact already doing the right thing on NativeAOT by checking "is effectively sealed" (meaning no subclasses)

so the getExactClasses == 1 check in this pr should be completely redundant

@AndyAyersMS
Copy link
Member

Seems like we ought to put the calls to getExactClasses into impIsClassExact to centralize the logic, and then just have these methods simply call impIsClassExact if they don't have exact classes.

As far as I know that's not needed because impIsClassExact already doing the right thing on NativeAOT by checking "is effectively sealed" (meaning no subclasses)

so the getExactClasses == 1 check in this pr should be completely redundant

So (perhaps I'm finally understanding what you were saying above) the only change needed is to one call to impClassIsExact

@MichalPetryka can you please try just running with that one change?

@MichalPetryka
Copy link
Contributor Author

@MihuBot -hetzner

@AndyAyersMS
Copy link
Member

The x86 failure here is a bit concerning. We are crashing in the GC.

Assert failure(PID 32236 [0x00007dec], Thread: 10744 [0x29f8]): Consistency check failed: AV in clr at this callstack:
------
CORECLR! WKS::gc_heap::background_mark_simple + 0x21 (0x702b6364)
CORECLR! WKS::gc_heap::background_promote + 0x109 (0x702b6a79)
CORECLR! PinObject + 0x91 (0x702807f1)
CORECLR! ScanConsecutiveHandlesWithoutUserData + 0x44 (0x7027fab4)
CORECLR! BlockScanBlocksWithoutUserData + 0x29 (0x7027f5e9)
CORECLR! ProcessScanQNode + 0x32 (0x7027f982)

@MichalPetryka
Copy link
Contributor Author

The x86 failure here is a bit concerning. We are crashing in the GC.

I don't recall that being there before, could it maybe be the rebase on top of main that caused this?

@kasperk81
Copy link
Contributor

    diff is a regression.
    relative diff is a regression.
262 improved, 1904 regressed

why is this acceptable?

@AndyAyersMS
Copy link
Member

The x86 failure here is a bit concerning. We are crashing in the GC.

Assert failure(PID 32236 [0x00007dec], Thread: 10744 [0x29f8]): Consistency check failed: AV in clr at this callstack:
------
CORECLR! WKS::gc_heap::background_mark_simple + 0x21 (0x702b6364)
CORECLR! WKS::gc_heap::background_promote + 0x109 (0x702b6a79)
CORECLR! PinObject + 0x91 (0x702807f1)
CORECLR! ScanConsecutiveHandlesWithoutUserData + 0x44 (0x7027fab4)
CORECLR! BlockScanBlocksWithoutUserData + 0x29 (0x7027f5e9)
CORECLR! ProcessScanQNode + 0x32 (0x7027f982)

@dotnet/gc any thoughts on this? I can load up the dump but can't get SOS to work.

@AndyAyersMS
Copy link
Member

    diff is a regression.
    relative diff is a regression.
262 improved, 1904 regressed

why is this acceptable?

We are expecting this change to unblock more devirtualization and inlining, so some size increases are expected.

@AndyAyersMS
Copy link
Member

    diff is a regression.
    relative diff is a regression.
262 improved, 1904 regressed

why is this acceptable?

We are expecting this change to unblock more devirtualization and inlining, so some size increases are expected.

Also we expect the size increases in practice will be smaller since many of those same inlines are already happening under PGO-driven GDV. But we can't see that impact easily right now.

Once this change is in and we do new SPMI collections we can undo it and get a more accurate read on the size impact.

@mangod9
Copy link
Member

mangod9 commented Jul 13, 2023

the test failure appears to be a GC hole, so might be useful to try with HeapVerify enabled to check if repros consistently.

@AndyAyersMS
Copy link
Member

the test failure appears to be a GC hole, so might be useful to try with HeapVerify enabled to check if repros consistently.

Let me see if it repros in CI.

@AndyAyersMS
Copy link
Member

I can't seem to get rerun to actually rerun the failing test.

@MichalPetryka please push a merge commit once #88856 is in so we can start from scratch.

@EgorBo
Copy link
Member

EgorBo commented Jul 16, 2023

/azp run runtime-coreclr outerloop, runtime-extra-platforms, runtime-coreclr pgostress

@azure-pipelines
Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@AndyAyersMS
Copy link
Member

Going to bounce this...

@AndyAyersMS AndyAyersMS reopened this Jul 18, 2023
@AndyAyersMS
Copy link
Member

Actually let me merge this up.

@EgorBo EgorBo merged commit be86455 into dotnet:main Jul 18, 2023
125 checks passed
@AndyAyersMS
Copy link
Member

@MichalPetryka thank you!

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