fix(cli): improve performance of linked library resolution during deployment #3197
Conversation
🦋 Changeset detectedLatest commit: 0bce659 The changes in this PR will be included in the next version bump. This PR includes changesets to release 26 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
| const library = libraries.find((lib) => lib.path === placeholder.path && lib.name === placeholder.name); | ||
| const libraryKey = getLibraryKey(placeholder); | ||
| const library = libraryMap[libraryKey]; |
There was a problem hiding this comment.
Previously we were doing a linear search to find the matching library every time is function was called, now we're just looking up the matching library in constant time. Doesn't make a big difference with small numbers of libraries but compounds with many libraries.
| bytecode = spliceHex( | ||
| bytecode, | ||
| placeholder.start, | ||
| placeholder.length, | ||
| library.prepareDeploy(deployer, libraries).address, | ||
| ); | ||
|
|
||
| // Store the prepared address in the library map to avoid preparing the same library twice | ||
| library.address ??= library.prepareDeploy(deployer, libraryMap).address; | ||
|
|
||
| bytecode = spliceHex(bytecode, placeholder.start, placeholder.length, library.address); |
There was a problem hiding this comment.
Previously we were resolving each library multiple times if it was used in multiple contracts/libraries. The time complexity of this quickly explodes if there are many libraries that use other libraries. Now we're caching a library's resolved address, so each library is only resolved once.
packages/cli/src/runDeploy.ts
Outdated
| @@ -133,6 +133,7 @@ export async function runDeploy(opts: DeployOptions): Promise<WorldDeploy> { | |||
| : undefined, | |||
| }), | |||
| account, | |||
| pollingInterval: 100, | |||
There was a problem hiding this comment.
Reducing polling time for a short time during deployment should be safe and makes a significant impact on deployment time (with the project i was testing with this change brought the time from 16s to 4s after the previous improvements were implemented)
There was a problem hiding this comment.
(moved this to waitForTransactions instead to only increase the polling rate where it matters)
There was a problem hiding this comment.
Moved this change to #3198 since it's unrelated to library linking, and CI wasn't happy with it yet
alvrs
force-pushed
the
alvrs/prepare-deploy
branch
from
88028b7
to
c816de9
Compare
| } | ||
|
|
||
| export function getLibraryKey({ path, name }: { path: string; name: string }): string { | ||
| return `${path}:${name}`; |
There was a problem hiding this comment.
should we include the deployer address in the key since it changes the bytecode and thus the address?
There was a problem hiding this comment.
the deployer address is passed in later (in prepareDeploy) while the map is created before, but we can change address in the map to { [deployer: Address]: Address } to cache per deployer address
| throw new Error("world deploy failed"); | ||
| } | ||
|
|
||
| const [receipt] = await waitForTransactions({ client, hashes: [tx], debugLabel: "world deploy" }); |
There was a problem hiding this comment.
iirc I intentionally didn't replace this one with waitForTransactions because I wanted to throw a more specific error, but this is fine too
There was a problem hiding this comment.
reason i changed this was bc i changed polling settings in waitForTransactions and would have had to duplicate the changes here, but moved that to #3198 so could also move this change there
packages/cli/src/verify.ts
Outdated
| @@ -105,7 +105,7 @@ export async function verify({ | |||
| ); | |||
|
|
|||
| modules.map(({ name, prepareDeploy }) => { | |||
| const { address } = prepareDeploy(deployerAddress, []); | |||
| const { address } = prepareDeploy(deployerAddress, {}); | |||
There was a problem hiding this comment.
thoughts on defaulting the second arg in prepareDeploy so callers don't need to know the shape of the thing to pass in
| export type LibraryMap = { [key: string]: Library & { address?: Hex } }; | ||
|
|
||
| export function getLibraryMap(libraries: readonly Library[]): LibraryMap { | ||
| return Object.fromEntries(libraries.map((library) => [getLibraryKey(library), library])); |
There was a problem hiding this comment.
not blocking but I wonder if we should just return some interface with get and set and leave the internal representation and key shape as implementation details, then you can use this almost like a regular map but with 2-3 args for the key (path + name + maybe deployer address)
Significantly improved the deployment performance for large projects with public libraries by implementing a more efficient algorithm to resolve public libraries during deployment.
The local deployment time on a large reference project was reduced from over 10 minutes to 4 seconds.