-
Notifications
You must be signed in to change notification settings - Fork 7
feat: ⚡️ Improve Caching Performance for multi runs #531
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
base: main
Are you sure you want to change the base?
Changes from all commits
3df2966
29c3abb
9165dfa
d11bde5
f429bd7
8e61820
3e32621
143cbac
52afad3
7d9739a
c998ce0
e17a42b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -6,21 +6,23 @@ import type { | |||||||||||||
| LaunchOverrides, | ||||||||||||||
| ForkConfig, | ||||||||||||||
| } from "@moonwall/types"; | ||||||||||||||
| import chalk from "chalk"; | ||||||||||||||
| import { createLogger } from "@moonwall/util"; | ||||||||||||||
| import path from "node:path"; | ||||||||||||||
| import net from "node:net"; | ||||||||||||||
| import { Effect } from "effect"; | ||||||||||||||
| import { standardRepos } from "../lib/repoDefinitions"; | ||||||||||||||
| import { shardManager } from "../lib/shardManager"; | ||||||||||||||
| import invariant from "tiny-invariant"; | ||||||||||||||
| import { StartupCacheService, StartupCacheServiceLive } from "./effect/StartupCacheService.js"; | ||||||||||||||
|
|
||||||||||||||
| const logger = createLogger({ name: "commandParsers" }); | ||||||||||||||
|
|
||||||||||||||
| export function parseZombieCmd(launchSpec: ZombieLaunchSpec) { | ||||||||||||||
| if (launchSpec) { | ||||||||||||||
| return { cmd: launchSpec.configPath }; | ||||||||||||||
| } | ||||||||||||||
| throw new Error( | ||||||||||||||
| `No ZombieSpec found in config. \n Are you sure your ${chalk.bgWhiteBright.blackBright( | ||||||||||||||
| "moonwall.config.json" | ||||||||||||||
| )} file has the correct "configPath" in zombieSpec?` | ||||||||||||||
| "No ZombieSpec found in config. Are you sure your moonwall.config.json file has the correct 'configPath' in zombieSpec?" | ||||||||||||||
| ); | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
|
|
@@ -122,8 +124,8 @@ export class LaunchCommandParser { | |||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| private print() { | ||||||||||||||
| console.log(chalk.cyan(`Command to run is: ${chalk.bold(this.cmd)}`)); | ||||||||||||||
| console.log(chalk.cyan(`Arguments are: ${chalk.bold(this.args.join(" "))}`)); | ||||||||||||||
| logger.debug(`Command to run: ${this.cmd}`); | ||||||||||||||
| logger.debug(`Arguments: ${this.args.join(" ")}`); | ||||||||||||||
| return this; | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
|
|
@@ -143,6 +145,99 @@ export class LaunchCommandParser { | |||||||||||||
| } | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| /** | ||||||||||||||
| * Cache startup artifacts if enabled in launchSpec. | ||||||||||||||
| * This uses an Effect-based service that caches artifacts by binary hash. | ||||||||||||||
| * | ||||||||||||||
| * When cacheStartupArtifacts is enabled, this generates: | ||||||||||||||
| * 1. Precompiled WASM for the runtime | ||||||||||||||
| * 2. Raw chain spec to skip genesis WASM compilation | ||||||||||||||
| * | ||||||||||||||
| * This reduces startup from ~3s to ~200ms (~10x improvement). | ||||||||||||||
|
||||||||||||||
| * This reduces startup from ~3s to ~200ms (~10x improvement). | |
| * This reduces startup from ~3s to ~200ms (~15x improvement). |
Copilot
AI
Dec 5, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The regex pattern --chain[=\s]?(\S+) used to extract the chain name may not handle all valid chain argument formats correctly. Specifically:
- The pattern allows optional whitespace or
=, but if whitespace is matched, it won't be consumed, causing the chain name to start with a space - The pattern uses
\S+which captures any non-whitespace, but this could capture additional arguments if--chainis followed by a space and the next argument (e.g.,--chain moonbase-dev --other-flagcould capturemoonbase-dev)
Consider using a more precise pattern:
const existingChainName = chainArg?.includes('=')
? chainArg.split('=')[1]
: this.args[this.args.indexOf(chainArg) + 1];Or if keeping the regex approach:
const existingChainName = chainArg?.match(/--chain(?:=(\S+)|\s+(\S+))/)?.[1] || chainArg?.match(/--chain(?:=(\S+)|\s+(\S+))/)?.[2];| const existingChainName = chainArg?.match(/--chain[=\s]?(\S+)/)?.[1]; | |
| const existingChainName = chainArg | |
| ? (chainArg.includes("=") | |
| ? chainArg.split("=")[1] | |
| : this.args[this.args.indexOf(chainArg) + 1]) | |
| : undefined; |
Copilot
AI
Dec 5, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When replacing --dev with a raw chain spec, the code adds --alice and --force-authoring flags using .push() instead of overrideArg(). This could lead to duplicate flags if these arguments already exist in the args array (e.g., from default args or user-specified options).
Use the overrideArg() method consistently to prevent duplicates:
this.overrideArg("--alice");
this.overrideArg("--force-authoring");However, note that overrideArg() may not handle flag-style arguments without = correctly (see the implementation at line 70-78), which could also cause issues.
Copilot
AI
Dec 5, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Setting process.env.MOONWALL_CACHE_DIR as a side effect during argument parsing could lead to unexpected behavior in multi-environment or concurrent test scenarios. Environment variables are global to the process, so this could cause cache directory conflicts when running multiple test suites in parallel.
Consider alternatives:
- Pass the cache directory through a different mechanism (e.g., a context object or dedicated configuration)
- Use a namespaced environment variable that includes the test environment ID or process ID
- Document this limitation clearly if parallel execution with different cache directories is not supported
Uh oh!
There was an error while loading. Please reload this page.