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

JIT: add option to use interlocked add for PGO edge count updates #81934

Merged
merged 1 commit into from
Feb 10, 2023
Merged

JIT: add option to use interlocked add for PGO edge count updates #81934

merged 1 commit into from
Feb 10, 2023

Conversation

AndyAyersMS
Copy link
Member

Mainly intended for use in determining what is leading to some consistency issues with our current edge profiles. We might consider enabling this in some of our static PGO collections.

Enabled via JitInterlockedProfile=1.

@AndyAyersMS
JIT: add option to use interlocked add for PGO edge count updates
a702534 
Mainly intended for use in determining what is leading to some consistency issues
with our current edge profiles. We might consider enabling this in some of our
static PGO collections.

Enabled via JitInterlockedProfile=1.
@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 Feb 10, 2023
@ghost ghost assigned AndyAyersMS Feb 10, 2023
@ghost
Copy link

ghost commented Feb 10, 2023

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

Issue Details

Mainly intended for use in determining what is leading to some consistency issues with our current edge profiles. We might consider enabling this in some of our static PGO collections.

Enabled via JitInterlockedProfile=1.

Author: AndyAyersMS
Assignees: AndyAyersMS
Labels:

area-CodeGen-coreclr
Milestone: -

@AndyAyersMS
Copy link
Member Author

@EgorBo PTAL
cc @dotnet/jit-contrib

Off by default, so no diffs/impact expected.

@AndyAyersMS AndyAyersMS mentioned this pull request Feb 10, 2023
Closed
44 tasks
@AndyAyersMS
Copy link
Member Author

Note the codegen here could be improved to lock inc [addr] but I don't know if it is worth polishing. I suppose we could see something similar for incrementing some class statics.

@build-analysis build-analysis bot mentioned this pull request Feb 10, 2023
Closed
EgorBo
EgorBo approved these changes Feb 10, 2023
View reviewed changes
Copy link
Member

@EgorBo EgorBo left a comment

Choose a reason for hiding this comment

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

Do we also need it for BasicBlockInstrumentator?

@EgorBo
Copy link
Member

EgorBo commented Feb 10, 2023 •
edited
Loading

It is interesting that the effect from this is not as bad as I expected on TE,

275k RPS (atomic) vs 285k RPS (default). But probably because it's hidden under cache ping-pong overhead

@AndyAyersMS
Copy link
Member Author

It is interesting that the effect from this is not as bad as I expected on TE,

275k RPS (atomic) vs 285k RPS (default). But probably because it's hidden under cache ping-pong overhead

Is that with or without ##81932?

@EgorBo
Copy link
Member

EgorBo commented Feb 10, 2023

It is interesting that the effect from this is not as bad as I expected on TE,
275k RPS (atomic) vs 285k RPS (default). But probably because it's hidden under cache ping-pong overhead

Is that with or without ##81932?

without

@AndyAyersMS AndyAyersMS merged commit ac2341c into dotnet:main Feb 10, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Mar 13, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@EgorBo EgorBo EgorBo approved these changes

Assignees

@AndyAyersMS AndyAyersMS

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.
2 participants
@AndyAyersMS @EgorBo