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

Fix logic error in PGO schema comparison #52822

Merged
merged 3 commits into from
May 17, 2021

Conversation

AndyAyersMS
Copy link
Member

I found this while trying to figure out what was leading to #51908.

I don't think this is the problem as SchemaMergesItemsWithDifferentOtherFields currently always returns false.

@AndyAyersMS
Copy link
Member Author

@davidwrighton PTAL

@jakobbotsch
Copy link
Member

jakobbotsch commented May 17, 2021

The meaning seems to be flipped. If the schema merges items different other fields, then they should always be considered equal. Otherwise they should be considered equal only if the Other fields are equal. (EDIT: Or maybe I misunderstand the meaning of the function name...)

But I don't think that's the check we want anyway. It seems like this code might be causing a lot of problems with edges. Right now, if multiple .mibc files have profile data for the same method, we end up merging all edges outgoing from the same basic block into one edge. So we lose practically all edge counts.

We should really just be checking if the instrumentation kinds are edges: jakobbotsch@45bcb3f
(this also fixing merging of 64-bit and 32-bit counts at the same time). There are lots of diffs when comparing the current .mibc we consume with a new one when we do that.

@AndyAyersMS
Copy link
Member Author

Or maybe I misunderstand the meaning of the function name.

I agree the name is confusing.

I think your change is clearer and also clearly improves behavior, so I'll update this PR to use that instead.

@AndyAyersMS
Copy link
Member Author

Failure looks like an instance of #52710.

@davidwrighton can you should review this one?

Copy link
Member

@davidwrighton davidwrighton left a comment

Choose a reason for hiding this comment

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

Long term, this code needs to be owned and understood most by the JIT team PGO experts, so if this looks good to both of you, I'm happy with whatever structure you like.

@davidwrighton davidwrighton merged commit c465b1c into dotnet:main May 17, 2021
@karelz karelz added this to the 6.0.0 milestone May 20, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Jun 19, 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.

4 participants