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

clang-tidy: added the ability to shard jobs #37265

Merged
auto-submit merged 17 commits into from
Nov 7, 2022
Merged

clang-tidy: added the ability to shard jobs #37265

Changes from 1 commit
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
20f3910
clang-tidy: added the ability to shard jobs
gaaclarke Nov 2, 2022
ee8469e
added test
gaaclarke Nov 3, 2022
56ab558
jenn feedback
gaaclarke Nov 3, 2022
e458963
hack ci to run as a shard to measure the time
gaaclarke Nov 4, 2022
e7c58b1
tweak
gaaclarke Nov 4, 2022
06a61a6
fix hack
gaaclarke Nov 4, 2022
e8527b3
zach feedback
gaaclarke Nov 4, 2022
664deb7
zach feedback 2
gaaclarke Nov 4, 2022
62a6607
removed stray async
gaaclarke Nov 4, 2022
d968147
moved to using sets for lookups
gaaclarke Nov 4, 2022
fa65733
fixed typo in docstring
gaaclarke Nov 4, 2022
a1438ab
Revert "fix hack"
gaaclarke Nov 7, 2022
72c6a87
removed calls to map
gaaclarke Nov 7, 2022
0d53794
turned the ci hack back on
gaaclarke Nov 7, 2022
e33c6a1
Revert "turned the ci hack back on"
gaaclarke Nov 7, 2022
bb2ded4
Merge remote-tracking branch 'upstream/main' into clang-tidy-shard
gaaclarke Nov 7, 2022
b68d266
removed sync*
gaaclarke Nov 7, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
zach feedback
  • Loading branch information
@gaaclarke
gaaclarke committed Nov 4, 2022
commit e8527b3bfcd4f2cba409697886baa255ca7b8cb3
8 changes: 6 additions & 2 deletions tools/clang_tidy/lib/clang_tidy.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -189,6 +189,7 @@ class ClangTidy {
return repo.changedFiles;
}

/// Returns f(n) = value(n * [shardCount] + [id]).
Iterable<T> _takeShard<T>(Iterable<T> values, int id, int shardCount) sync* {
Copy link
Member

@zanderso zanderso Nov 4, 2022

Choose a reason for hiding this comment

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

I think the code from here down that is reasoning about sets of commands could be made a bit more idiomatic, and maybe more readable as well, by encapsulating it in a new class (Commands, CommandSet, or CommandShard, or something) that can live in lib/src/command.dart.

Copy link
Member Author

@gaaclarke gaaclarke Nov 4, 2022

Choose a reason for hiding this comment

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

Respectfully, I don't think that will help make the code easier to read or maintain. The small codebase already has poor oop design already and it actually made my job harder. In objective terms adding more classes will result in more lines of code, more segmentation of the code, and more concepts you need to know to modify the code.

As an example, there is no reason for CommandSet to exist when Set<Command> (or List<Command>) already encapsulates the idea perfectly. People can come into the codebase and know what that is immediately, not so with CommandSet. I also can't understand from what the difference between a CommandSet and a CommandShard is.

Copy link
Member

@zanderso zanderso Nov 4, 2022

Choose a reason for hiding this comment

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

As an example, there is no reason for CommandSet to exist when Set<Command> (or List<Command>) already encapsulates the idea perfectly.

The code in getLintCommandsForFiles is not operating on the Set, List, etc. in its full generality. Instead it's doing something very specific: trying to filter unwanted items out of a big set. The specific internal data structures that are used to accomplish that goal are an implementation detail. This code would be easier to read and maintain if the the implementation details were encapsulated somehow. It doesn't necessarily have to be a class. It could be more helper methods, for example. I don't have a strong opinion there. However, I think even with comments, the code in getLintCommandsForFiles could be improved on, and that is overall what I'm asking for.

I also can't understand from what the difference between a CommandSet and a CommandShard is.

These were just naming suggestions for a single new class.

Copy link
Member Author

@gaaclarke gaaclarke Nov 4, 2022

Choose a reason for hiding this comment

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

Yea, I think the documentation could be better and will help with that. I've updated the docstring in getLintCommandsForFiles and added one for _takeShard.

int count = 0;
for (final T val in values) {
Expand Down Expand Up @@ -222,8 +223,11 @@ class ClangTidy {
}
}

/// Given a build commands json file, and the files with local changes,
/// compute the lint commands to run.
/// 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>> getLintCommandsForFiles(
List<Object?> buildCommandsData,
Expand Down