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

format_tool: optimization pass #424

Merged
merged 4 commits into from
Feb 21, 2024
Merged

format_tool: optimization pass #424

merged 4 commits into from
Feb 21, 2024

Conversation

alexandercampbell-wk
Copy link
Contributor

We can save a lot (~90%) of execution time of dart_dev format on some repos with two changes:

  • Not recursing into directories that are a priori excluded from the formatter's list (e.g., hidden directories like .git). Note that this means we no longer build and log a comprehensive list of excluded files. I think it's worth it to run so much faster.

  • Removing the collapseDirectories feature, to be replaced with batch parallel invocations. Computing the collapsible directories was some kind of superlinear deduplication. This could maybe be optimized in a way that would allow it to remain, but it seems it was intended as a stopgap solution for argv limitations in the first place.

This is technically a breaking change to the API since we no longer return a full list of excluded files and hidden directories.
I have retained the collapseDirectories parameter in the name of backward compatibility for now, but it's ignored and marked @deprecated.

This moves the responsibility for handling argv limitations into FormatTool.

We can save a lot (90%) of execution time on some repos with two
changes:

 - Not recursing into directories that are a priori excluded from the
   formatter's list (e.g., hidden directories like .git). Note that this
   means we no longer build and log a comprehensive list of excluded
   files. I think it's worth it to run so much faster.

 - Removing the collapseDirectories feature, to be replaced with
   batch parallel invocations. Computing the collapsible directories
   was some kind of superlinear deduplication. This could maybe be
   optimized in a way that would allow it to remain, but it seems it was
   intended as a stopgap solution for argv limitations in the first
   place.
@aviary2-wf
Copy link

Security Insights

No security relevant content was detected by automated scans.

Action Items

  • Review PR for security impact; comment "security review required" if needed or unsure
  • Verify aviary.yaml coverage of security relevant code

Questions or Comments? Reach out on Slack: #support-infosec.

Copy link
Contributor

@evanweible-wf evanweible-wf left a comment

Choose a reason for hiding this comment

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

LGTM! Feel free to add the filter functions as alternatives to globs in this PR or a separate one.

lib/src/tools/format_tool.dart Outdated Show resolved Hide resolved
@alexandercampbell-wk
Copy link
Contributor Author

I just pushed a commit to restore the deleted fields back onto FormatterInputs, but leave them unused and marked as @deprecated. I believe that was the only Dart API breaking change. I want to be extra careful about compat here given the dependency loops between tools.

@alexandercampbell-wk
Copy link
Contributor Author

Oops, I broke CI. I'll get that addressed today.

@alexandercampbell-wk alexandercampbell-wk marked this pull request as ready for review February 21, 2024 21:04

if (entry is Link) {
_log.finer('skipping link $relative\n');
skippedLinks.add(relative);
_log.finer('skipping link $filename\n');
Copy link
Member

Choose a reason for hiding this comment

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

Why did these checks not make it into the listSync forEach? Would they even be faster there?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That would definitely be faster— I chose to keep this closer to the original logic for now to avoid churn. I tentatively plan to make another pass to reduce the amount of work being done in this function later.

@alexandercampbell-wk
Copy link
Contributor Author

@Workiva/release-management-pp

@rmconsole5-wk rmconsole5-wk merged commit 7deb814 into master Feb 21, 2024
8 checks passed
Copy link
Contributor

@rmconsole-wf rmconsole-wf left a comment

Choose a reason for hiding this comment

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

+1 from RM

@rmconsole5-wk rmconsole5-wk deleted the optimize-format-tool branch February 21, 2024 22:42
@tomconnell-wf
Copy link
Member

Rosie doesn't require QA on this repo? :sus:

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.

6 participants