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

chore(cargo): aim for faster runtime on release profile #594

Merged

Conversation

mkniewallner
Copy link
Collaborator

@mkniewallner mkniewallner commented Mar 15, 2024

PR Checklist

  • A description of the changes is added to the description of this PR.
  • If there is a related issue, make sure it is linked to this PR.
  • If you've fixed a bug or added code that should be tested, add tests!
  • Documentation in docs is updated

Description of changes

Per https://nnethercote.github.io/perf-book/build-configuration.html, there are a few optimisations we could apply to the artifacts we build when releasing.

Testing locally on macOS ARM, this lead to:

  • ~5% runtime performance improvements when running deptry on a repository with ~7k files (from ~1.02s to ~970ms)
  • ~20% reduction on the binary size (from 2.0M to 1.6M)

The tradeoff being that we will have longer compilation times, but since this should only impact the "release" profile and not the dev one, and we do not create releases that often, I think this should be an ok tradeoff.

Copy link

codecov bot commented Mar 15, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.7%. Comparing base (95477b9) to head (48a338d).

Additional details and impacted files
@@          Coverage Diff          @@
##            main    #594   +/-   ##
=====================================
  Coverage   90.7%   90.7%           
=====================================
  Files         38      38           
  Lines       1086    1086           
  Branches     224     224           
=====================================
  Hits         986     986           
  Misses        85      85           
  Partials      15      15           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@fpgmaas fpgmaas self-requested a review March 15, 2024 12:42
@mkniewallner
Copy link
Collaborator Author

mkniewallner commented Mar 15, 2024

The changes heavily increase tests duration. This is because the installation of deptry made in functional tests uses the "release" profile and not the dev one, and since we build deptry from source for each test environment (as we want proper isolation), each environment setup now takes more time to be bootstrapped.

I guess we could either:

  • reduce the number of times we need to build deptry wheel, by sharing it across multiple tests projects (Speed up functional tests #597)
  • force the dev profile to be used during tests

I'm not a huge fan of the 2nd option because:

  • tests would still be slow
  • we would not test against the same binary that end up being released, as dev builds coud have some differences if we have different configurations

So I'll probably give a shot at the first option. In the meantime, I'll let the PR in draft, but if we decide that we're ok with the increased test duration in the meantime, I'm also ok with merging this PR as is.

@mkniewallner mkniewallner marked this pull request as ready for review March 16, 2024 01:09
@mkniewallner
Copy link
Collaborator Author

Will rebase from main once #598 is merged to assess the new duration increase.

@fpgmaas fpgmaas self-requested a review March 16, 2024 08:30
@fpgmaas
Copy link
Owner

fpgmaas commented Mar 16, 2024

Will rebase from main once #598 is merged to assess the new duration increase.

Awesome, let's see what that does to the unit test duration in the CI/CD pipeline so we can assess if we think the increased runtime peformance for end-users outweighs the increased unit test duration.

@mkniewallner mkniewallner force-pushed the chore/faster-runtime-release-builds branch from 01054f3 to 48a338d Compare March 16, 2024 08:52
@mkniewallner
Copy link
Collaborator Author

After the rebase, it's day and night when comparing latest build with https://github.com/fpgmaas/deptry/actions/runs/8295732898. Will let you confirm that @fpgmaas but to me this looks good to merge now.

@mkniewallner mkniewallner merged commit d8790e1 into fpgmaas:main Mar 16, 2024
29 checks passed
@mkniewallner mkniewallner deleted the chore/faster-runtime-release-builds branch March 16, 2024 09:48
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