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

[MONO] Add JIT_CODE_DEBUG_INFO record functionality for Jitdump #85107

Merged
merged 8 commits into from
May 17, 2023

Conversation

saitama951
Copy link
Contributor

@saitama951 saitama951 commented Apr 20, 2023

This is related to #36774 . added support for JIT_CODE_DEBUG_INFO record which helps to annotate source code in perf.

Changed the jitdump version to 1 as the perf sources expect version 1

https://github.com/torvalds/linux/blob/master/tools/perf/util/jitdump.h#L25

Example use:-
MONO_ENV_OPTIONS="--jitdump" perf record -k 1 dotnet <path/to/binary>
perf inject --jit -i perf.data -o perf.jit.data
perf report -i perf.jit.data

cc: @fanyang-mono

@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Apr 20, 2023
@@ -2128,7 +2145,75 @@ mono_emit_jit_dump (MonoJitInfo *jinfo, gpointer code)

record.code_index = ++code_index;

// TODO: write debugInfo and unwindInfo immediately before the JitCodeLoadRecord (while lock is held).
DebugEntry ent;
Copy link
Contributor

Choose a reason for hiding this comment

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

This should follow the mono coding conventions, see the rest of the file for examples.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done. let me know if there is anything else

Copy link
Contributor Author

@saitama951 saitama951 left a comment

Choose a reason for hiding this comment

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

changed the style to adhere to the mono coding conventions

@@ -2128,7 +2145,75 @@ mono_emit_jit_dump (MonoJitInfo *jinfo, gpointer code)

record.code_index = ++code_index;

// TODO: write debugInfo and unwindInfo immediately before the JitCodeLoadRecord (while lock is held).
DebugEntry ent;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

done. let me know if there is anything else

@saitama951 saitama951 requested a review from vargaz April 21, 2023 04:52
Copy link
Member

@fanyang-mono fanyang-mono left a comment

Choose a reason for hiding this comment

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

Please go through your change and fix the coding style. I've pointed out a few examples for you.

guint32 line;
guint32 discrim;
char name[];
}DebugEntry;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
}DebugEntry;
} DebugEntry;

guint64 code_addr;
guint64 nr_entry;
DebugEntry debug_entry[];
}JitCodeDebug;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
}JitCodeDebug;
} JitCodeDebug;


static void add_basic_JitCodeDebug_info(JitCodeDebug *record);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
static void add_basic_JitCodeDebug_info(JitCodeDebug *record);
static void add_basic_JitCodeDebug_info (JitCodeDebug *record);

Copy link
Member

Choose a reason for hiding this comment

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

In mono code base, we add a space between the function name and the "(". Please follow this function call style for all the code change in this PR.

rec.code_addr = (guint64)dmji->code_start;
rec.header.total_size = sizeof(rec) + sizeof(ent) + 1;
rec.nr_entry=1;
for(i=0;i < dmji->num_line_numbers;++i){
Copy link
Member

Choose a reason for hiding this comment

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

Fix code style


loc = mono_debug_lookup_source_location_by_il(jinfo->d.method,dmji->line_numbers[i].il_offset,NULL);

if(!(loc)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if(!(loc)
if(!loc)

@saitama951
Copy link
Contributor Author

Please go through your change and fix the coding style. I've pointed out a few examples for you.

Thank you for helping me out with the code style. I have made the changes accordingly.

@saitama951
Copy link
Contributor Author

saitama951 commented May 4, 2023

@fanyang-mono is there any other changes required to be made?
maybe we can merge the code?

@directhex
Copy link
Member

@vargaz @lambdageek can someone see if this is ready to merge now?

@fanyang-mono fanyang-mono merged commit a5a485c into dotnet:main May 17, 2023
@shivanirmishra
Copy link

shivanirmishra commented May 25, 2023

@fanyang-mono
The fix did not work for me.
The steps we tried are:

1) dotnet build
2) MONO_ENV_OPTIONS="--jitmap --jitdump" perf record -k 1 dotnet run
3) perf inject --jit -i perf.data -o perf.jit.data
4) perf report -i perf.jit.data

It's creating multiple sets of .map and .dump files in the /tmp/ folder for one particular workload.

Error:
objdump: /tmp/perf-886285.map: File format not recognized

Perf tool is able to read .map files to do symbol resolution and show the data. And due to this, we are able to see the functions in the profile but not the annotations inside that which requires a .dump file.

@saitama951
Copy link
Contributor Author

saitama951 commented May 26, 2023

@shivanirmishra The runtime should generate the .dump file. can you cross check that under the /tmp/?
if that is present can you check if the dumps are being read by using the strace?

I faced a similar issue and fixed the same : #82520

@shivanirmishra
Copy link

Hi @saitama951, yes runtime is generating the .dump file in the/tmp/ folder.
But Strace is not showing any references to dump files.
The observations we had are with .NET7.0 so is the above fix available only on .NET8 or later?

@saitama951
Copy link
Contributor Author

@shivanirmishra the fix should be there in .NET 8 preview 4 or you can cherry pick the commit.

@shivanirmishra
Copy link

@saitama951
We tried with .NET8 Preview 4, but getting the below issue after using perf inject:
image
Also when we are doing Strace on the same, we can see the MMAP calls there:
image
So, what can cause this wrong jitdump version 2, expected 1 issue in perf inject?

@saitama951
Copy link
Contributor Author

saitama951 commented May 31, 2023

@shivanirmishra I have fixed the wrong versioning of jitdump in the current PR itself, probably this fix isn't incorporated in this release I guess , I would suggest you can build dotnet from source that should work fine.

@shivanirmishra
Copy link

Hi @saitama951, thank you for your help.
This fix is working for the sample dotnet code.
But while building dotnet for the workload from https://github.com/dotnet/performance, we are facing the below issues with the power system:
image
Any suggestion regarding the above error?
Also, I have changed the target frameworks to .net8 from .net7.

@fanyang-mono
Copy link
Member

/backport to release/7.0-staging

@github-actions
Copy link
Contributor

Started backporting to release/7.0-staging: https://github.com/dotnet/runtime/actions/runs/5248244227

@shivanirmishra
Copy link

@saitama951 @fanyang-mono
We are facing the above issue on .net8 on power, so will this backporting to 7.0 fix the issue on .net8 as well?

@saitama951
Copy link
Contributor Author

@shivanirmishra It seems source code annotation fails on top of the BenchmarkDotnet library. it works for sample programs without the BDN. I have been manually looking into the jitted-so's, everything looks good there as well.

@shivanirmishra
Copy link

Thanks @saitama951
It works for other workloads that run for a shorter duration.
But if we are trying to run a longer workload, then sometimes it's showing "unknown" in the profile on both X86 and Power systems.

For Example: after running the BinaryTrees_5 workload from https://github.com/dotnet/performance, we get the below profiles.

Before Inject:
image
After Inject:
image

@fanyang-mono
Copy link
Member

@saitama951 @fanyang-mono We are facing the above issue on .net8 on power, so will this backporting to 7.0 fix the issue on .net8 as well?

Yes, I am working on backporting the fixes to regressions to 7.0 now.

@fanyang-mono
Copy link
Member

@shivanirmishra We will get this fix in the next possible servicing release.

@shivanirmishra
Copy link

@saitama951 @fanyang-mono
The above fix is working for some applications but not for all.
For example, It's not working if we increase the no of iterations for the same application

This is the profile with 100 iterations of some ml.net workload:

image

This is the profile with 10k iterations of the same workload:

image

The above profiles are after perf inject.

Any suggestions or ideas for why this is not working with larger overhead?

@saitama951
Copy link
Contributor Author

@shivanirmishra I have proposed the fix for the issue that you were facing.

#88373

@shivanirmishra
Copy link

Hi @saitama951 @fanyang-mono
This is another sample code for matrix multiplication, in which we are seeing the same above issue.

MONO_ENV_OPTIONS="--jitmap --jitdump" perf record -k 1 /home/user/dotnet run

Dimension of the matrix: 2000x3 and 3x2000, and running the MultiplyMatrices function once.

perf Report -i perf.data

image

perf inject -j -i ./perf.data -o ./perf.jit.data

perf report -i perf.jit.data

image

We are not able to see the main function MultiplyMatrices anywhere in the profile.
And wherever we are seeing the project name (new-matrix) as a command in the profile, we don’t see any functions/annotations for some so it might be splitting the main function into some parts and we are not able to resolve the same.

image

@saitama951
Copy link
Contributor Author

saitama951 commented Jul 4, 2023

@shivanirmishra have you applied the diff changes ?

@shivanirmishra
Copy link

@saitama951 Yes, It's working.
Thank you.

@ghost ghost locked as resolved and limited conversation to collaborators Aug 5, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-Codegen-JIT-mono 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