Skip to content

Commit

Permalink
clang-tidy: added the ability to shard jobs (flutter#37265)
Browse files Browse the repository at this point in the history
* clang-tidy: added the ability to shard jobs

* added test

* jenn feedback

* hack ci to run as a shard to measure the time

* tweak

* fix hack

* zach feedback

* zach feedback 2

* removed stray async

* moved to using sets for lookups

* fixed typo in docstring

* Revert "fix hack"

This reverts commit 06a61a6.

Revert "tweak"

This reverts commit e7c58b1.

Revert "hack ci to run as a shard to measure the time"

This reverts commit e458963.

* removed calls to map

* turned the ci hack back on

* Revert "turned the ci hack back on"

This reverts commit 0d53794.

* removed sync*
  • Loading branch information
gaaclarke authored and schwa423 committed Nov 16, 2022
1 parent f94ee45 commit 4909830
Show file tree
Hide file tree
Showing 3 changed files with 238 additions and 31 deletions.
136 changes: 115 additions & 21 deletions tools/clang_tidy/lib/clang_tidy.dart
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,17 @@ class _ComputeJobsResult {
final bool sawMalformed;
}

enum _SetStatus {
Intersection,
Difference,
}

class _SetStatusCommand {
_SetStatusCommand(this.setStatus, this.command);
final _SetStatus setStatus;
final Command command;
}

/// A class that runs clang-tidy on all or only the changed files in a git
/// repo.
class ClangTidy {
Expand Down Expand Up @@ -92,14 +103,14 @@ class ClangTidy {

_outSink.writeln(_linterOutputHeader);

final List<io.File> changedFiles = await computeChangedFiles();
final List<io.File> filesOfInterest = await computeFilesOfInterest();

if (options.verbose) {
_outSink.writeln('Checking lint in repo at ${options.repoPath.path}.');
if (options.checksArg.isNotEmpty) {
_outSink.writeln('Checking for specific checks: ${options.checks}.');
}
final int changedFilesCount = changedFiles.length;
final int changedFilesCount = filesOfInterest.length;
if (options.lintAll) {
_outSink.writeln('Checking all $changedFilesCount files the repo dir.');
} else {
Expand All @@ -109,12 +120,19 @@ class ClangTidy {
}
}

final List<dynamic> buildCommandsData = jsonDecode(
final List<Object?> buildCommandsData = jsonDecode(
options.buildCommandsPath.readAsStringSync(),
) as List<dynamic>;
final List<Command> changedFileBuildCommands = await getLintCommandsForChangedFiles(
) as List<Object?>;
final List<List<Object?>> shardBuildCommandsData = options
.shardCommandsPaths
.map((io.File file) =>
jsonDecode(file.readAsStringSync()) as List<Object?>)
.toList();
final List<Command> changedFileBuildCommands = await getLintCommandsForFiles(
buildCommandsData,
changedFiles,
filesOfInterest,
shardBuildCommandsData,
options.shardId,
);

if (changedFileBuildCommands.isEmpty) {
Expand Down Expand Up @@ -153,7 +171,7 @@ class ClangTidy {
/// The files with local modifications or all the files if `lintAll` was
/// specified.
@visibleForTesting
Future<List<io.File>> computeChangedFiles() async {
Future<List<io.File>> computeFilesOfInterest() async {
if (options.lintAll) {
return options.repoPath
.listSync(recursive: true)
Expand All @@ -171,23 +189,99 @@ class ClangTidy {
return repo.changedFiles;
}

/// Given a build commands json file, and the files with local changes,
/// compute the lint commands to run.
/// Returns f(n) = value(n * [shardCount] + [id]).
Iterable<T> _takeShard<T>(Iterable<T> values, int id, int shardCount) sync* {
int count = 0;
for (final T val in values) {
if (count % shardCount == id) {
yield val;
}
count++;
}
}

/// This returns a `_SetStatusCommand` for each [Command] in [items].
/// `Intersection` if the Command shows up in [items] and its filePath in all
/// [filePathSets], otherwise `Difference`.
Iterable<_SetStatusCommand> _calcIntersection(
Iterable<Command> items, Iterable<Set<String>> filePathSets) sync* {
bool allSetsContain(Command command) {
for (final Set<String> filePathSet in filePathSets) {
if (!filePathSet.contains(command.filePath)) {
return false;
}
}
return true;
}
for (final Command command in items) {
if (allSetsContain(command)) {
yield _SetStatusCommand(_SetStatus.Intersection, command);
} else {
yield _SetStatusCommand(_SetStatus.Difference, command);
}
}
}

/// Given a build commands json file's contents in [buildCommandsData], and
/// the [files] with local changes, compute the lint commands to run. If
/// build commands are supplied in [sharedBuildCommandsData] the intersection
/// of those build commands will be calculated and distributed across
/// instances via the [shardId].
@visibleForTesting
Future<List<Command>> getLintCommandsForChangedFiles(
List<dynamic> buildCommandsData,
List<io.File> changedFiles,
) async {
final List<Command> buildCommands = <Command>[];
for (final dynamic data in buildCommandsData) {
final Command command = Command.fromMap(data as Map<String, dynamic>);
final LintAction lintAction = await command.lintAction;
// Short-circuit the expensive containsAny call for the many third_party files.
if (lintAction != LintAction.skipThirdParty && command.containsAny(changedFiles)) {
buildCommands.add(command);
Future<List<Command>> getLintCommandsForFiles(
List<Object?> buildCommandsData,
List<io.File> files,
List<List<Object?>> sharedBuildCommandsData,
int? shardId,
) {
final List<Command> totalCommands = <Command>[];
if (sharedBuildCommandsData.isNotEmpty) {
final List<Command> buildCommands = <Command>[
for (Object? data in buildCommandsData)
Command.fromMap((data as Map<String, Object?>?)!)
];
final List<Set<String>> shardFilePaths = <Set<String>>[
for (List<Object?> list in sharedBuildCommandsData)
<String>{
for (Object? data in list)
Command.fromMap((data as Map<String, Object?>?)!).filePath
}
];
final Iterable<_SetStatusCommand> intersectionResults =
_calcIntersection(buildCommands, shardFilePaths);
for (final _SetStatusCommand result in intersectionResults) {
if (result.setStatus == _SetStatus.Difference) {
totalCommands.add(result.command);
}
}
final List<Command> intersection = <Command>[
for (_SetStatusCommand result in intersectionResults)
if (result.setStatus == _SetStatus.Intersection) result.command
];
// Make sure to sort results so the sharding scheme is guaranteed to work
// since we are not sure if there is a defined order in the json file.
intersection
.sort((Command x, Command y) => x.filePath.compareTo(y.filePath));
totalCommands.addAll(
_takeShard(intersection, shardId!, 1 + sharedBuildCommandsData.length));
} else {
totalCommands.addAll(<Command>[
for (Object? data in buildCommandsData)
Command.fromMap((data as Map<String, Object?>?)!)
]);
}
return buildCommands;
return () async {
final List<Command> result = <Command>[];
for (final Command command in totalCommands) {
final LintAction lintAction = await command.lintAction;
// Short-circuit the expensive containsAny call for the many third_party files.
if (lintAction != LintAction.skipThirdParty &&
command.containsAny(files)) {
result.add(command);
}
}
return result;
}();
}

Future<_ComputeJobsResult> _computeJobs(
Expand Down
56 changes: 50 additions & 6 deletions tools/clang_tidy/lib/src/options.dart
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,8 @@ class Options {
this.fix = false,
this.errorMessage,
this.warningsAsErrors,
this.shardId,
this.shardCommandsPaths = const <io.File>[],
StringSink? errSink,
}) : checks = checksArg.isNotEmpty ? '--checks=$checksArg' : null,
_errSink = errSink ?? io.stderr;
Expand Down Expand Up @@ -64,6 +66,8 @@ class Options {
ArgResults options, {
required io.File buildCommandsPath,
StringSink? errSink,
required List<io.File> shardCommandsPaths,
int? shardId,
}) {
return Options(
help: options['help'] as bool,
Expand All @@ -76,6 +80,8 @@ class Options {
fix: options['fix'] as bool,
errSink: errSink,
warningsAsErrors: _platformSpecificWarningsAsErrors(options),
shardCommandsPaths: shardCommandsPaths,
shardId: shardId,
);
}

Expand All @@ -87,25 +93,42 @@ class Options {
final ArgResults argResults = _argParser.parse(arguments);

String? buildCommandsPath = argResults['compile-commands'] as String?;

String variantToBuildCommandsFilePath(String variant) =>
path.join(
argResults['src-dir'] as String,
'out',
variant,
'compile_commands.json',
);
// path/to/engine/src/out/variant/compile_commands.json
buildCommandsPath ??= path.join(
argResults['src-dir'] as String,
'out',
argResults['target-variant'] as String,
'compile_commands.json',
);
buildCommandsPath ??= variantToBuildCommandsFilePath(argResults['target-variant'] as String);
final io.File buildCommands = io.File(buildCommandsPath);
final List<io.File> shardCommands =
(argResults['shard-variants'] as String? ?? '')
.split(',')
.where((String element) => element.isNotEmpty)
.map((String variant) =>
io.File(variantToBuildCommandsFilePath(variant)))
.toList();
final String? message = _checkArguments(argResults, buildCommands);
if (message != null) {
return Options._error(message, errSink: errSink);
}
if (argResults['help'] as bool) {
return Options._help(errSink: errSink);
}
final String? shardIdString = argResults['shard-id'] as String?;
final int? shardId = shardIdString == null ? null : int.parse(shardIdString);
if (shardId != null && (shardId >= shardCommands.length || shardId < 0)) {
return Options._error('Invalid shard-id value: $shardId.', errSink: errSink);
}
return Options._fromArgResults(
argResults,
buildCommandsPath: buildCommands,
errSink: errSink,
shardCommandsPaths: shardCommands,
shardId: shardId,
);
}

Expand All @@ -132,6 +155,17 @@ class Options {
'verbose',
help: 'Print verbose output.',
)
..addOption(
'shard-id',
help: 'When used with the shard-commands option this identifies which shard will execute.',
valueHelp: 'A number less than 1 + the number of shard-commands arguments.',
)
..addOption(
'shard-variants',
help: 'Comma separated list of other targets, this invocation '
'will only execute a subset of the intersection and the difference of the '
'compile commands. Use with `shard-id`.'
)
..addOption(
'compile-commands',
help: 'Use the given path as the source of compile_commands.json. This '
Expand Down Expand Up @@ -171,6 +205,12 @@ class Options {
/// The location of the compile_commands.json file.
final io.File buildCommandsPath;

/// The location of shard compile_commands.json files.
final List<io.File> shardCommandsPaths;

/// The identifier of the shard.
final int? shardId;

/// The root of the flutter/engine repository.
final io.Directory repoPath = io.Directory(_engineRoot);

Expand Down Expand Up @@ -233,6 +273,10 @@ class Options {
return "ERROR: Build commands path ${buildCommandsPath.absolute.path} doesn't exist.";
}

if (argResults.wasParsed('shard-variants') && !argResults.wasParsed('shard-id')) {
return 'ERROR: a `shard-id` must be specified with `shard-variants`.';
}

return null;
}
}
Loading

0 comments on commit 4909830

Please sign in to comment.