Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Conversation

@jmagman
Copy link
Member

@jmagman jmagman commented Nov 10, 2021

Significantly speed up the clang-tidy script. The changed files/compile-commands intersection containsAny calculation is very expensive. Short-circuit this call for the third_party files that would only have been skipped later.

Reduced --lint-all ios_debug on my machine from 11:54 minutes to 2:28.

Optimize for linting all files in a target with flutter/flutter#61661.

Pre-launch Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I read and followed the Flutter Style Guide and the C++, Objective-C, Java style guides.
  • I listed at least one issue that this PR fixes in the description above.
  • I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test-exempt. See testing the engine for instructions on
    writing and running engine tests.
  • I updated/added relevant documentation (doc comments with ///).
  • I signed the CLA.
  • All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

Copy link
Member

@zanderso zanderso left a comment

Choose a reason for hiding this comment

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

@jmagman jmagman changed the title Speed up clang-tidy script by skipping missing files Speed up clang-tidy script by skipping third_party files Nov 10, 2021
@jmagman
Copy link
Member Author

jmagman commented Nov 10, 2021

Huh. So is this code https://github.com/flutter/engine/blob/master/tools/clang_tidy/lib/clang_tidy.dart#L157 including files that don't exist?

I guess I had run ./flutter/tools/gn to get host_debug awhile ago (and the script doesn't re-run it if it already exists), I was missing matrix_decomposition* removed in #29530

Really, the speed up is in skipping the expensive command.containsAny(changedFiles) for third_party files that will be skipped anyway.
I updated the title and description to make that clearer.

for (final dynamic data in buildCommandsData) {
final Command command = Command.fromMap(data as Map<String, dynamic>);
final LintAction lintAction = await command.lintAction;
if (lintAction != LintAction.skipMissing &&
Copy link
Member Author

Choose a reason for hiding this comment

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

Skip files that don't exist in case there is anything stale in host_debug/compile_commands.json.

}

final io.File file = io.File(filePath);
if (!file.existsSync()) {
Copy link
Member Author

Choose a reason for hiding this comment

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

Without this check reading the lines crashes

final Stream<String> lines = file.openRead()

@jmagman
Copy link
Member Author

jmagman commented Nov 10, 2021

Huh. So is this code https://github.com/flutter/engine/blob/master/tools/clang_tidy/lib/clang_tidy.dart#L157 including files that don't exist?

Also, no, the repo list check only includes files that exist, and changedFiles only includes files that exist. So command.containsAny(changedFiles) successfully returns false for missing files. It's the command that may point to a file that doesn't exist. That containsAny call is really expensive, and if we avoid calling it for third_party files that won't be linted anyway, then it speeds the script up significantly. The skipMissing is just to be resilient and not crash when parsing missing files checking for FLUTTER_NOLINT.

final Stream<String> lines = file.openRead()

'--compile-commands',
// This just has to exist.
io.Platform.executable,
io.Platform.script.path,
Copy link
Member Author

Choose a reason for hiding this comment

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

io.Platform.executable crashed, this file is actually parsed now and needs to be utf8.

Copy link
Member

Choose a reason for hiding this comment

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

Could you update the comment on the line above?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

Copy link
Member

@zanderso zanderso left a comment

Choose a reason for hiding this comment

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

LGTM

'--compile-commands',
// This just has to exist.
io.Platform.executable,
io.Platform.script.path,
Copy link
Member

Choose a reason for hiding this comment

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

Could you update the comment on the line above?

);
const String filePath = '/path/to/a/source_file.cc';

// This just has to exist.
Copy link
Member

Choose a reason for hiding this comment

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

ditto

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

@jmagman jmagman added the waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. label Nov 10, 2021
@fluttergithubbot fluttergithubbot merged commit 874d36d into flutter:master Nov 10, 2021
@jmagman jmagman deleted the faster-clang-tidy branch November 10, 2021 19:16
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 10, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

cla: yes waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants