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

Delete second checkPackageDescription call #10516

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

parsonsmatt
Copy link
Collaborator

While diagnosing slow check times, I identified that file glob expansion was very slow, even on literal paths (#10495). This was made worse because Cabal would expand a glob four times for each extra-source-files line.

Fortunately, we can halve this easily: we call checkPackageDescription twice in checkGenericPackageDescription. This PR simply removes one of the calls in the function.

On our codebase, this patch has the effect of reducing the time spent expanding globs from 34s to 17s, bringing the time to do cabal repl --repl-options "-e ()" from 50s to 33s.

Template Α: This PR modifies behaviour or interface

Include the following checklist in your PR:

  • Patches conform to the coding conventions.
  • Any changes that could be relevant to users have been recorded in the changelog.
  • The documentation has been updated, if necessary.
  • Manual QA notes have been included.
  • Tests have been added. (Ask for help if you don’t know how to write them! Ask for an exemption if tests are too complex for too little coverage!)

Copy link
Collaborator Author

@parsonsmatt parsonsmatt left a comment

Choose a reason for hiding this comment

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

Copy link
Collaborator

@ffaf1 ffaf1 left a comment

Choose a reason for hiding this comment

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

Very good find.

@ffaf1
Copy link
Collaborator

ffaf1 commented Nov 4, 2024

I don't think this requires a Template A, as it fixes a bug.

Copy link
Collaborator

@ulysses4ever ulysses4ever left a comment

Choose a reason for hiding this comment

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

Thanks!

Excuse us if we're slow in other PRs of yours: it's a release time for us. But we'll get there.

@parsonsmatt
Copy link
Collaborator Author

Y'all are reviewing these quickly, no worries at all! 😄 We will most likely be using a patched version while we wait for relesae, so there's no rush on our end.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants