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

Replace perf with faster patched built-from-source version #198

Closed
wants to merge 0 commits into from

Conversation

glpuga
Copy link
Collaborator

@glpuga glpuga commented May 25, 2023

Proposed changes

Replaces the perf installation with a built-from-source version that's patched to be much, much faster. This will make it much easier to analyze and compare the performance of different versions. After this patch, the flamegraph takes about 8 minutes to be generated on an Intel(R) Core(TM) i7-10750H CPU @ 2.60GHz.

The patched version is the one described in here.

Type of change

  • 🐛 Bugfix (change which fixes an issue)
  • 🚀 Feature (change which adds functionality)
  • 📚 Documentation (change which fixes or extends documentation)

💥 Breaking change! Explain why a non-backwards compatible change is necessary or remove this line entirely if not applicable.

Checklist

Put an x in the boxes that apply. This is simply a reminder of what we will require before merging your code.

  • Lint and unit tests (if any) pass locally with my changes
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)
  • All commmits have been signed for DCO

Additional comments

While I could have opted for not installing linux-tools-generic in the first place, instead of installing and the removing before patching, I decided to execute the process from the same starting point used in the instructions in flamegraph-rs/flamegraph#74 (comment) , which assume linux-tools-generic (and the packages it depends on) to have been previously installed.

Reference results

The performance results for both the likelihood and beam configurations can be seen below.

In all cases, the results were obtained by profiling e72e148 with the step-by-step instructions from PROFILING.md.

The computer is an Intel(R) Core(TM) i7-10750H CPU @ 2.60GHz.

Beam sensor model

flamegraph_beam

Likelihood sensor model

flamegraph_likelihood

@glpuga glpuga force-pushed the glpuga/speed_up_perf branch from 5a29bdc to 2cf5f8e Compare May 25, 2023 21:11
@glpuga glpuga marked this pull request as ready for review May 25, 2023 21:12
@glpuga glpuga requested review from hidmic and nahueespinosa May 25, 2023 21:14
@glpuga glpuga force-pushed the glpuga/speed_up_perf branch from 2cf5f8e to a865907 Compare May 25, 2023 22:27
@@ -0,0 +1,920 @@
// SPDX-License-Identifier: GPL-2.0
Copy link
Collaborator

Choose a reason for hiding this comment

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

@glpuga the GPL is pervasive, quoting:

  1. You may modify your copy or copies of the Program or any portion of it [...] provided that you also meet all of these conditions:
    b) You must cause any work that you distribute or publish, that in whole or in part contains or is derived from the Program or any part thereof, to be licensed as a whole at no charge to all third parties under the terms of this License.

There is a notion of separate work in GPL terms and conditions, but I'm no lawyer, and this smells as a liability. How about we relocate this to a different repository? A minimal Github Actions workflow to push to Dockerhub should do to make it available to Beluga.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hmm... good catch. I flew totally past me checking the license implications of adding the file replacement.

This is not essential, so I'll take down this PR while we figure if it's worth adding it back in some other form.

@glpuga glpuga closed this May 26, 2023
@glpuga glpuga force-pushed the glpuga/speed_up_perf branch from a865907 to e72e148 Compare May 26, 2023 02:23
@glpuga glpuga deleted the glpuga/speed_up_perf branch May 26, 2023 02:23
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.

2 participants