Compile the `dart_dev` run script for better performance #426

alexandercampbell-wk · 2024-03-05T19:23:06Z

This PR adapted from a 2021 PR by @evanweible-wf: #363. See that PR for discussion and performance metrics.

My changes:

Merge conflicts resolved.
Most code review comments addressed.
Misc cleanup and rearrangement to bring this branch in line with since-merged refactors (most notably the refactor of path usage performed in Fixes for Windows compatibility #386; specifically 1ad8316).

Functionality changes described below.

After compilation, the startup time for subsequent dart run dart_dev commands should be significantly improved.

This compiled exectuable is cached in .dart_tool/dart_dev/ along with a digest that is used to determine when recompilation is needed.

Packages with tool/dart_dev/config.dart files that import from their own package's source files will be opted out of this compilation optimization because there is no efficient way to track those imports for the purpose of detecting the need to recompile.

This commit also adds a clean subcommand to dart run dart_dev that will remove the files in .dart_tool/dart_dev/, in case a bug is found in our compilation caching logic.

After compilation, the startup time for subsequent `dart run dart_dev` commands should be significantly improved. This compiled exectuable is cached in `.dart_tool/dart_dev/` along with a digest that is used to determine when recompilation is needed. Packages with `tool/dart_dev/config.dart` files that import from their own package's source files will be opted out of this compilation optimization because there is no efficient way to track those imports for the purpose of detecting the need to recompile. This commit also adds a `clean` subcommand to `dart run dart_dev` that will remove the files in `.dart_tool/dart_dev/`, in case a bug is found in our compilation caching logic.

aviary3-wk · 2024-03-05T19:24:14Z

Security Insights

The items listed below may not capture all security relevant changes. Before providing a security review, be sure to review the entire PR for security impact.

(1) Security relevant changes were detected

Watched keyword md5. in lib/src/executable.dart line(s) ['161'] added

Action Items

Obtain a security review; reviewer should pay special attention to insights listed above
Verify aviary.yaml coverage of security relevant code

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

Also run `ddev format` to fix wrapping in executable.dart.

I think this will fix the CI failure on Windows specifically.

greglittlefield-wf · 2024-03-06T16:38:40Z

Thanks for reviving this @alexandercampbell-wk!! I'm super excited on about this performance optimization, and had been meaning to circle back to it for a while.

At a glance these changes look good, but I can do a more thorough review later today.

greglittlefield-wf

A couple comments and some optional-to-address #nits

Tested a little bit locally and seems to work great!

lib/src/executable.dart

greglittlefield-wf · 2024-03-06T19:21:06Z

lib/src/executable.dart

+      return;
+    }
+
+    final digest = md5.convert([


If we're allowing imports of other packages, I think we should include something in the digest that causes recompilation when dependencies change. E.g., the contents of .dart_tool/package_config.json.

Oh, it looks ilke that was present in the previous PR: https://github.com/Workiva/dart_dev/pull/363/files#diff-16bd3bf901e6560bcc074b4d1fdc6750f4c1ab19007f4d6f0d1acfa0989b9f08R160
Could we reinstate that?

Also, there's this related comment about skipping caching for mutable dependencies: https://github.com/Workiva/dart_dev/pull/363/files#r753702320

Feels like it'd be worth addressing; @evanweible-wf what do you think?

Yeah +1 to both of these comments - even if it's rare for mutable deps to be imported and used in ddev config, it'd be a very confusing issue to debug, so it'd be nice to cover this.

I have added the contents of .dart_tool/package_config.json to the md5 digest here.

Handling caching for mutable dependencies, based on package_config, pubspec.yaml, and pubspec.lock, sounds like a more complex task. I'll have to follow up on this next week. Thank you both for the careful review.

@evanweible-wf @greglittlefield-wf I have addressed this comment. It took me an hour or so to implement but it is working well now.

Sadly, the main repo where I was interested in using this precompilation system does have a path dependency in the imports for the config file, meaning that this safety change now forces recompilation on every run :/

I can think of some workarounds (tree walking + statting for timestamps?), but nothing quick. I think this PR is probably good to go for now and we could try to think of a more sophisticated solution in the future.

lib/src/tools/clean_tool.dart

evanweible-wf

Just one minor thing, but LGTM.

evanweible-wf · 2024-03-18T14:35:35Z

lib/src/executable.dart

+  final pubSpecLock = loadYamlDocument(pubspecLockFile.readAsStringSync());
+  return getDependencySources(pubSpecLock, packageNames)
+      .values
+      .every((source) => source == 'hosted');


I just realized this makes it hard to test the behavior locally because any custom tool/dart_dev/config.dart will always be importing dart_dev from a local or git source. We should be able to consider git sources immutable, too, since the package config file included in the digest below will include the resolved ref of each git source.

I've updated this line to consider 'hosted' and 'git' immutable.

alexandercampbell-wk · 2024-03-25T18:14:43Z

@evanweible-wf @greglittlefield-wf this is ready for re-review at your convenience.

evanweible-wf · 2024-03-25T18:43:01Z

QA +1

Installed this branch locally in a dart project
Verified that the run script is compiled and cached and subsequent runs do not recompile
Verified that the run script is recompiled when a change is detected

evanweible-wf · 2024-03-25T18:43:42Z

+1 security

Use of md5 has no security impact; it is used only to compute a digest for the purposes of caching the run script

Changes reviewed by Evan

alexandercampbell-wk · 2024-04-01T17:32:43Z

@Workiva/release-management-pp

tomconnell-wf · 2024-04-01T18:46:30Z

lib/src/executable.dart

+  final process = await Process.start(
+      processArgs.first,
+      [
+        if (processArgs.length > 1) ...processArgs.sublist(1),
+        ...args,
+      ],


Security review: parameterized shelling out is good.

tomconnell-wf · 2024-04-01T18:52:34Z

lib/src/executable.dart

+    [_paths.runExecutable, _paths.runExecutableDigest].forEach((p) {
+      final f = File(p);
+      if (f.existsSync()) f.deleteSync();
+    });


Security review: The files to delete look like they are statically defined, so this couldn't be used to delete something unexpected: https://github.com/Workiva/dart_dev/pull/426/files#diff-6e5f65bcac0e9f8da735ef58d443983df38419788614afe4a860adebb5016f5cR35-R37

tomconnell-wf · 2024-04-01T18:55:28Z

lib/src/executable.dart

+      final args = [
+        'compile',
+        'exe',
+        _paths.runScript,
+        '-o',
+        _paths.runExecutable
+      ];
+      final result = Process.runSync(Platform.executable, args);


Security review: parameterized shelling 👍

tomconnell-wf · 2024-04-01T18:57:27Z

lib/src/executable.dart

+        // user that dart_dev should still work.
+        log.warning(
+            'Could not compile run script; dart_dev will continue without precompilation.');
+        log.fine('CMD: ${Platform.executable} ${args.join(" ")}');


Security review: afaik, logging out the args given isn't problematic on cli.

tomconnell-wf · 2024-04-01T19:02:35Z

lib/src/dart_dev_runner.dart

 import 'utils/version.dart';

 class DartDevRunner extends CommandRunner<int> {
  DartDevRunner(Map<String, DevTool> commands)
      : super('dart_dev', 'Dart tool runner.') {
+    // For backwards-compatibility, only add the `clean` command if it doesn't
+    // conflict with any configured command.
+    if (!commands.containsKey('clean')) {


Nice touch.

tomconnell-wf · 2024-04-01T19:04:07Z

lib/src/executable.dart

This file is starting to have a lot of responsibilities. I could have seen the new digest-related functions in a separate file. 🤷

tomconnell-wf · 2024-04-01T19:09:47Z

lib/src/executable.dart

+
+/// Return null iff it is not possible to account for all
+/// recompilation-necessitating factors in the digest.
+String? _computeRunScriptDigest() {


It would have been nice to split this function up to call a config getter, configImports validator, and digest encoder function so it was easier to digest.

tomconnell-wf · 2024-04-01T19:11:56Z

lib/src/executable.dart

+    // Digest is missing or outdated, so we (re-)compile.
+    final logMessage = runExecutable.existsSync()
+        ? 'Recompiling run script (digest changed)'
+        : 'Compiling run script';
+    logTimedSync(log, logMessage, () {
+      // Delete the previous executable and digest so that if we hit a failure
+      // trying to compile, we don't leave the outdated one in place.
+      _deleteRunExecutableAndDigest();
+      final args = [
+        'compile',
+        'exe',
+        _paths.runScript,
+        '-o',
+        _paths.runExecutable
+      ];
+      final result = Process.runSync(Platform.executable, args);
+      if (result.exitCode == 0) {
+        // Compilation succeeded. Write the new digest, as well.
+        runExecutableDigest.writeAsStringSync(encodedDigest!);
+      } else {
+        // Compilation failed. Dump some logs for debugging, but note to the
+        // user that dart_dev should still work.
+        log.warning(
+            'Could not compile run script; dart_dev will continue without precompilation.');
+        log.fine('CMD: ${Platform.executable} ${args.join(" ")}');
+        log.fine('STDOUT:\n${result.stdout}');
+        log.fine('STDERR:\n${result.stderr}');
+      }


It would have been nice to split this out into a separate function. 🤷

tomconnell-wf · 2024-04-01T19:14:46Z

lib/src/tools/clean_tool.dart

+
+  @override
+  FutureOr<int?> run([DevToolExecutionContext? context]) {
+    final cache = Directory(DartDevPaths().cache());


Security review: hardcoded to delete .dart-tool/dart_dev

tomconnell-wf · 2024-04-01T19:15:42Z

lib/src/utils/parse_imports.dart

+
+/// Return the contents of the enquoted portion of the import statements in the
+/// file. Not 100% accurate, since we use regular expressions instead of the
+/// Dart AST to extract the imports.


pfft, not that Dart's AST parsing would be 100% accurate either. 😉

tomconnell-wf

To a dart_dev layperson familiar with the problem you're solving, this looks reasonable.

tomconnell-wf · 2024-04-01T19:19:39Z

Security +1

I don't see anything that can make dart_dev do something that it isn't expected to do.

rmconsole-wf

+1 from RM

rmconsole-wf added the Merge Requirements Unmet label Mar 5, 2024

aviary2-wf added the Security Review Required label Mar 5, 2024

rmconsole-wf added the Skynet 2.0 Failed label Mar 5, 2024

alexandercampbell-wk mentioned this pull request Mar 5, 2024

Compile the dart_dev run script for better performance #363

Closed

alexandercampbell-wk added 3 commits March 5, 2024 13:50

DartDevRunner: workaround type failure

a400944

Also run `ddev format` to fix wrapping in executable.dart.

executable: remove File suffix from File vars

69af6f2

executable: use absolute path to runExecutable

b395a77

I think this will fix the CI failure on Windows specifically.

alexandercampbell-wk marked this pull request as ready for review March 5, 2024 21:53

alexandercampbell-wk requested review from evanweible-wf, todbachman-wf and greglittlefield-wf March 5, 2024 21:53

rmconsole-wf removed the Skynet 2.0 Failed label Mar 6, 2024

greglittlefield-wf previously requested changes Mar 6, 2024

View reviewed changes

alexandercampbell-wk added 7 commits March 8, 2024 11:33

generateRunScript: address code review comments

4f72e97

ddev format dart_dev_paths

b887017

Ensure packageConfig exists before reading

7ee6009

executable: refactor computeRunScriptDigest into its own function

04cef7f

Implement more robust import parser with unit test

d4c1a59

Parse pubspec.lock to check whether config imports are hosted

500199f

ddev format

2fc5874

evanweible-wf approved these changes Mar 18, 2024

View reviewed changes

evanweible-wf previously approved these changes Mar 18, 2024

View reviewed changes

Consider git-sourced dependencies immutable

3e0312f

alexandercampbell-wk dismissed evanweible-wf’s stale review via 3e0312f March 21, 2024 16:37

evanweible-wf approved these changes Mar 25, 2024

View reviewed changes

evanweible-wf added the semver: minor label Mar 25, 2024

rmconsole-wf added the RM Ready label Apr 1, 2024

tomconnell-wf reviewed Apr 1, 2024

View reviewed changes

tomconnell-wf approved these changes Apr 1, 2024

View reviewed changes

rmconsole-wf added Merge Requirements Met and removed Merge Requirements Unmet labels Apr 1, 2024

rmconsole-wf approved these changes Apr 1, 2024

View reviewed changes

rm-astro-wf merged commit 2ec310a into master Apr 1, 2024
8 checks passed

rm-astro-wf deleted the compile_run_script_for_better_perf_followup branch April 1, 2024 19:20

rmconsole-readonly-wk mentioned this pull request Sep 5, 2024

RM-242322 Release dart_dev 4.2.0 #433

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Compile the `dart_dev` run script for better performance #426

Compile the `dart_dev` run script for better performance #426

alexandercampbell-wk commented Mar 5, 2024

aviary3-wk commented Mar 5, 2024 •

edited by aviary-wf

Loading

greglittlefield-wf commented Mar 6, 2024

greglittlefield-wf left a comment

greglittlefield-wf Mar 6, 2024

greglittlefield-wf Mar 6, 2024

evanweible-wf Mar 8, 2024

alexandercampbell-wk Mar 8, 2024

alexandercampbell-wk Mar 12, 2024

evanweible-wf left a comment

evanweible-wf Mar 18, 2024

alexandercampbell-wk Mar 21, 2024

alexandercampbell-wk commented Mar 25, 2024

evanweible-wf commented Mar 25, 2024

evanweible-wf commented Mar 25, 2024

alexandercampbell-wk commented Apr 1, 2024

tomconnell-wf Apr 1, 2024

tomconnell-wf Apr 1, 2024

tomconnell-wf Apr 1, 2024

tomconnell-wf Apr 1, 2024

tomconnell-wf Apr 1, 2024

tomconnell-wf Apr 1, 2024

tomconnell-wf Apr 1, 2024

tomconnell-wf Apr 1, 2024

tomconnell-wf Apr 1, 2024

tomconnell-wf Apr 1, 2024

tomconnell-wf left a comment

tomconnell-wf commented Apr 1, 2024

rmconsole-wf left a comment

Compile the dart_dev run script for better performance #426

Compile the dart_dev run script for better performance #426

Conversation

alexandercampbell-wk commented Mar 5, 2024

aviary3-wk commented Mar 5, 2024 • edited by aviary-wf Loading

Security Insights

Action Items

greglittlefield-wf commented Mar 6, 2024

greglittlefield-wf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evanweible-wf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexandercampbell-wk commented Mar 25, 2024

evanweible-wf commented Mar 25, 2024

evanweible-wf commented Mar 25, 2024

alexandercampbell-wk commented Apr 1, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomconnell-wf left a comment

Choose a reason for hiding this comment

tomconnell-wf commented Apr 1, 2024

Security +1

rmconsole-wf left a comment

Choose a reason for hiding this comment

Compile the `dart_dev` run script for better performance #426

Compile the `dart_dev` run script for better performance #426

aviary3-wk commented Mar 5, 2024 •

edited by aviary-wf

Loading