Skip to content

[hooks_runner] Optimize the no hooks path #2302

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

Merged
merged 2 commits into from
May 16, 2025
Merged

Conversation

dcharkes
Copy link
Collaborator

@dcharkes dcharkes commented May 15, 2025

Bug: #2236

Optimize the code paths for when there are no hooks (speeds up standalone Dart).

  • Return early in various places, and check if there are hooks at all as first thing.
  • Write JSON encoding instead of YAML encoding. 17 ms -> 1 ms on a small JSON object. (And JSON is valid YAML.)

After this the longest running part in dartdev is loading the package_config.json: 10 ms. We can't really avoid this because we need that info to check whether we have any hooks that need running. And the package_config.json isn't already loaded by dartdev (in contrast to flutter_tools where we have a PackageConfig object available). dartdev delegates compilation to package:pub(!) via getExecutableForCommand and that API uses the package config file path. dart-lang/pub#4067

Copy link

PR Health

API leaks ✔️

The following packages contain symbols visible in the public API, but not exported by the library. Export these symbols or remove them from your publicly visible API.

Package Leaked API symbols
License Headers ✔️
// Copyright (c) 2025, the Dart project authors. Please see the AUTHORS file
// for details. All rights reserved. Use of this source code is governed by a
// BSD-style license that can be found in the LICENSE file.
Files
no missing headers

All source files should start with a license header.

Unrelated files missing license headers
Files
pkgs/jni/lib/src/third_party/generated_bindings.dart
pkgs/objective_c/lib/src/ns_input_stream.dart

2 similar comments
Copy link

PR Health

API leaks ✔️

The following packages contain symbols visible in the public API, but not exported by the library. Export these symbols or remove them from your publicly visible API.

Package Leaked API symbols
License Headers ✔️
// Copyright (c) 2025, the Dart project authors. Please see the AUTHORS file
// for details. All rights reserved. Use of this source code is governed by a
// BSD-style license that can be found in the LICENSE file.
Files
no missing headers

All source files should start with a license header.

Unrelated files missing license headers
Files
pkgs/jni/lib/src/third_party/generated_bindings.dart
pkgs/objective_c/lib/src/ns_input_stream.dart

Copy link

PR Health

API leaks ✔️

The following packages contain symbols visible in the public API, but not exported by the library. Export these symbols or remove them from your publicly visible API.

Package Leaked API symbols
License Headers ✔️
// Copyright (c) 2025, the Dart project authors. Please see the AUTHORS file
// for details. All rights reserved. Use of this source code is governed by a
// BSD-style license that can be found in the LICENSE file.
Files
no missing headers

All source files should start with a license header.

Unrelated files missing license headers
Files
pkgs/jni/lib/src/third_party/generated_bindings.dart
pkgs/objective_c/lib/src/ns_input_stream.dart

@dcharkes dcharkes force-pushed the hooks-runner-optimizations branch from 048ec58 to cc6fc7b Compare May 15, 2025 15:00
@dcharkes dcharkes force-pushed the hooks-runner-optimizations branch from cc6fc7b to 177bea1 Compare May 15, 2025 15:11
@coveralls
Copy link

coveralls commented May 15, 2025

Coverage Status

coverage: 83.667% (-8.0%) from 91.694%
when pulling 1e86a3b on hooks-runner-optimizations
into 5c7152e on main.

@dcharkes dcharkes requested a review from HosseinYousefi May 15, 2025 15:32
Copy link
Contributor

@goderbauer goderbauer left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -40,7 +41,7 @@ class KernelAssets {
},
};

return yamlEncode(yamlContents);
return const JsonEncoder.withIndent(' ').convert(yamlContents);
Copy link
Contributor

Choose a reason for hiding this comment

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

I can see a future reader stumbling over this line and wondering why we encode yaml with the Json encoder. Can you leave a comment or fix the naming?

@auto-submit auto-submit bot merged commit 6e0edca into main May 16, 2025
37 checks passed
@auto-submit auto-submit bot deleted the hooks-runner-optimizations branch May 16, 2025 08:02
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.

3 participants