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

Add gprof profiling support. #7547

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
Open

Conversation

Rossmaxx
Copy link
Contributor

@Rossmaxx Rossmaxx commented Oct 17, 2024

Adds gprof support for profiling the builds. Also removed the -fomit-frame-pointer flag from swh and tap suite as the flag interferes with the -pg flag used here.

per https://stackoverflow.com/questions/14666665/trying-to-understand-gcc-option-fomit-frame-pointer , this option makes getting stack traces nearly impossible, and the performance improvements seems to be negligible on 64 bit systems, especially when debugging.

to use gprof:
after building with the WANT_DEBUG_GPROF flag turned on, run lmms, and after running, gprof ./lmms gmon.out > report.txt from the build directory. This will generate a text file with the profiling data. Do note that this method is tested only on GCC, and windows builds might not be supported.

plugins/LadspaEffect/swh/CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
@Rossmaxx
Copy link
Contributor Author

I have updated the title description according to the recent changes.

@Rossmaxx Rossmaxx changed the title Improve debugging ability and add gprof profiling support. Add gprof profiling support. Oct 20, 2024
@JohannesLorenz
Copy link
Contributor

I was wondering if it makes sense to allow gprof even in Debug mode. At least, it turned out that the results were not so far from the Release mode results.

Still, I wonder if we should print a warning if gprof is active in Debug mode, so the user knows that the results may not be representative?

Copy link
Contributor

@JohannesLorenz JohannesLorenz left a comment

Choose a reason for hiding this comment

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

Approved.

What I did:

  • Functional review ✔️
  • Style review ✔️
  • Test review ✔️

@Rossmaxx
Copy link
Contributor Author

I'll merge this in 2 days if no one objects. I choose to merge this with 1 approval because this is a trivial PR and shouldn't have any impact other than the flag

@Rossmaxx
Copy link
Contributor Author

I was wondering if it makes sense to allow gprof even in Debug mode. At least, it turned out that the results were not so far from the Release mode results.

Still, I wonder if we should print a warning if gprof is active in Debug mode, so the user knows that the results may not be representative?

saw this now after I took another look. I could but I choose not to for now. I don't see any need to add a warning, building on debug mode, one should naturally know that the performance won't be good.

@@ -664,6 +665,20 @@ elseif(MSVC)
add_compile_options("/utf-8")
ENDIF()

# gcc builds support gprof for profiling
if(NOT CMAKE_CXX_COMPILER_ID MATCHES "GNU")
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

should modifying the matches string to "GNU|CLANG" suffice or is there any other syntax which I should be looking at?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have made the change for now, if it's wrong, just follow up on it and i'll change it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sakertooth can you test for me using clang if this syntax works and you can use gprof?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried to use it, but seems like the functionality is different under clang, or I'm not understanding something.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I asked gpt (don't judge me for this), here's what it said.

Recent versions of Clang may not fully support gprof. The -pg flag might be accepted but not functional, as gprof is increasingly considered outdated. Instead, Clang recommends using more modern profiling tools like perf, llvm-profdata, or llvm-cov.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am thinking of not adding clang support for now. If anyone needs it, we can add it later anyway. I hope everyone agrees with this.

CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants