-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
feat: implement the solidity compilation cache #6129
base: v-next
Are you sure you want to change the base?
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
1807993
to
a1d4dca
Compare
hardhatTotal size of the bundle: List of dependencies (sorted by size)
|
a1d4dca
to
ae3401d
Compare
ae3401d
to
db3ac09
Compare
db3ac09
to
73ba8e0
Compare
await Promise.all( | ||
compilationJobs.map(async (compilationJob) => | ||
compilationJob.getBuildId(), | ||
), | ||
); |
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.
Computing build IDs is time consuming as it involves hashing all the compiler input sources. That's why we do it asynchronously. Once a compilation job build ID is computed, it will be returned straight away on the subsequent requests.
// NOTE: We're not waiting for the writes and clean to finish because we | ||
// will only care about the result of these operations in subsequent runs | ||
void Promise.all( | ||
uncachedSuccessfulResults.map(async (result) => { | ||
return this.#compilerOutputCache.set( | ||
await result.compilationJob.getBuildId(), | ||
result.compilerOutput, | ||
); | ||
}), | ||
).then(() => { | ||
return this.#compilerOutputCache.clean(); | ||
}); |
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 should we clean up the cache was one of the open questions stated in the design doc.
To do so from the cleanArtifacts
method was suggested. I do like this suggestion as then all the "clean" operations would be grouped together. However, when working on the final implementation, I realised it might be more beneficial to call the cache clean from build after all.
As you can see in the snippet, this allows us to ensure that clean is only ever called after all the writes have finished. That way we can avoid (or at least postpone) implementing synchronisation mechanisms between writes and cleans in the cache component.
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.
Context
This is the implementation of the caching component.
It is expected to support caching compiler outputs to begin with. We can extend/change it as needed in the future.
defaultMaxAgeMs: number = 7 * 24 * 60 * 60 * 1000, // 1 week | ||
defaultMaxSize: number = 2 * 1024 * 1024 * 1024, // 2 GB |
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.
These are the default values we agreed upon here. They are not exposed to the user at the moment.
return (await exists(filePath)) ? readJsonFile<T>(filePath) : undefined; | ||
} | ||
|
||
public async clean(maxAgeMs?: number, maxSize?: number): Promise<void> { |
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.
Being able to override the default max age and size is really useful for testing purposes.
for (const fileInfo of sortedFileInfos) { | ||
if (fileInfo.atimeMs < minAtimeMs || size > maxSize) { | ||
filesToRemove.push(fileInfo.file); | ||
size -= fileInfo.size; | ||
} else { | ||
break; | ||
} | ||
} |
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.
We remove the files starting from the front of the list (the "oldest" entries) until the total size of the cache is smaller than the maximum cache size and the oldest entry is newer than the maximum age of a cache entry.
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.
Context
The changes in this file are concerned with the getBuildId
optimisations. They are related to the effort to make the getBuildId
asynchronous and to cache resolved file content hashes.
#solcInputWithoutSources: Omit<CompilerInput, "sources"> | undefined; | ||
#resolvedFiles: ResolvedFile[] | undefined; |
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.
By adding these two new properties on the compilation job, we ensure they are only ever computed once per compilation job.
const sortedSources = Object.fromEntries( | ||
Object.entries(sources).sort((a, b) => a[0].localeCompare(b[0])), | ||
); |
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.
We need to sort the sources because the sources map might be populated out of order which does affect serialisation.
buildId: string; | ||
contractArtifactsGenerated: string[]; | ||
warnings: CompilerOutputError[]; |
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.
Since we're storing the entire compiler outputs in the cache, we're able to extra warnings out of them even when they're retrieved from cache.
// eslint-disable-next-line @typescript-eslint/consistent-type-assertions -- We don't care about hooks in this context | ||
hooks.setContext({} as HookContext); |
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 we call build()
it is asserted that the hook context is not undefined
. We don't actually use hooks in this tests.
A proper HookContext
contains hre
. I didn't want to create a full hre
for this test. Hence, the type cast.
export async function createNonCryptographicHashId( | ||
data: string, | ||
): Promise<string> { | ||
const message = new TextEncoder().encode(data); | ||
const buffer = await crypto.subtle.digest("SHA-1", message); | ||
const array = Array.from(new Uint8Array(buffer)); | ||
return array.map((b) => b.toString(16).padStart(2, "0")).join(""); | ||
} |
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.
Context
The changes to this file are related to the getBuildId
optimisations.
After the changes, createNonCryptographicHashId
is an asynchronous function. I also changed the hashing algorithm from MD5 to SHA-1. This is OK because the only place where we currently use this function is the getBuildId
method of the CompilationJobImplementation
.
export async function getAccessTime(absolutePath: string): Promise<Date> { | ||
try { | ||
const stats = await fsPromises.stat(absolutePath); | ||
return stats.atime; | ||
} catch (e) { | ||
ensureError<NodeJS.ErrnoException>(e); | ||
if (e.code === "ENOENT") { | ||
throw new FileNotFoundError(absolutePath, e); | ||
} | ||
|
||
throw new FileSystemAccessError(e.message, e); | ||
} | ||
} |
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.
Context
Here, I add two new helper functions, getAccessTime
and getSize
which return last access time and size of a file respectively.
They are needed to implement the clean
part of the caching component which removes cache entries based on file size and "age".
@@ -104,9 +104,9 @@ declare module "@ignored/hardhat-vnext/types/artifacts" { | |||
}`; | |||
} | |||
|
|||
export function getBuildInfo( | |||
export async function getBuildInfo( |
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.
Context
It is necessary to change the signatures of getBuildInfo
and getBuildInfoOutput
because they both use getBuildId
which turned asynchronous as part of the getBuildId
optimisations.
|
||
import { ResolvedFileType } from "../../../../types/solidity.js"; | ||
|
||
export class ProjectResolvedFileImplementation implements ProjectResolvedFile { |
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.
Context
I add implementations of ProjectResolvedFile
and NpmPackageResolvedFile
interfaces here. These are necessary in order to lazily store content hashes on them, as proposed as part of the getBuildId
optimisations.
@@ -35,14 +35,15 @@ export interface ResolvedNpmPackage { | |||
*/ | |||
export enum ResolvedFileType { | |||
PROJECT_FILE = "PROJECT_FILE", | |||
NPM_PACKGE_FILE = "NPM_PACKAGE_FILE", | |||
NPM_PACKAGE_FILE = "NPM_PACKAGE_FILE", | |||
} | |||
|
|||
/** | |||
* A file that's part of the Hardhat project (i.e. not installed through npm). | |||
*/ | |||
export interface ProjectResolvedFile { |
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.
Context
I add getContentHash()
methods on the ProjectResolvedFile
and NpmPackageResolvedFile
interfaces here. These functions are needed to implement the getBuildId
optimisations.
In this PR, I implement the solidity compilation cache as outlined in the associated design doc.
The implementation diverges from the design doc in 1 areas: