From 4b4289a668a61a293d042a6c7aec2e76abdf4097 Mon Sep 17 00:00:00 2001 From: Danny van Velzen Date: Mon, 10 Mar 2025 21:22:52 -0700 Subject: [PATCH 1/4] Use RefCountedCache for ZipFs instances in Cache --- .yarn/versions/1137015d.yml | 34 ++++++ packages/yarnpkg-core/sources/Cache.ts | 22 +++- .../yarnpkg-core/sources/RefCountedCache.ts | 69 +++++++++++ .../tests/RefCountedCache.test.ts | 107 ++++++++++++++++++ 4 files changed, 228 insertions(+), 4 deletions(-) create mode 100644 .yarn/versions/1137015d.yml create mode 100644 packages/yarnpkg-core/sources/RefCountedCache.ts create mode 100644 packages/yarnpkg-core/tests/RefCountedCache.test.ts diff --git a/.yarn/versions/1137015d.yml b/.yarn/versions/1137015d.yml new file mode 100644 index 000000000000..ca82f129ff62 --- /dev/null +++ b/.yarn/versions/1137015d.yml @@ -0,0 +1,34 @@ +releases: + "@yarnpkg/cli": minor + "@yarnpkg/core": minor + +declined: + - "@yarnpkg/plugin-compat" + - "@yarnpkg/plugin-constraints" + - "@yarnpkg/plugin-dlx" + - "@yarnpkg/plugin-essentials" + - "@yarnpkg/plugin-exec" + - "@yarnpkg/plugin-file" + - "@yarnpkg/plugin-git" + - "@yarnpkg/plugin-github" + - "@yarnpkg/plugin-http" + - "@yarnpkg/plugin-init" + - "@yarnpkg/plugin-interactive-tools" + - "@yarnpkg/plugin-link" + - "@yarnpkg/plugin-nm" + - "@yarnpkg/plugin-npm" + - "@yarnpkg/plugin-npm-cli" + - "@yarnpkg/plugin-pack" + - "@yarnpkg/plugin-patch" + - "@yarnpkg/plugin-pnp" + - "@yarnpkg/plugin-pnpm" + - "@yarnpkg/plugin-stage" + - "@yarnpkg/plugin-typescript" + - "@yarnpkg/plugin-version" + - "@yarnpkg/plugin-workspace-tools" + - "@yarnpkg/builder" + - "@yarnpkg/doctor" + - "@yarnpkg/extensions" + - "@yarnpkg/nm" + - "@yarnpkg/pnpify" + - "@yarnpkg/sdks" diff --git a/packages/yarnpkg-core/sources/Cache.ts b/packages/yarnpkg-core/sources/Cache.ts index f0883c874437..15873c9eb6cb 100644 --- a/packages/yarnpkg-core/sources/Cache.ts +++ b/packages/yarnpkg-core/sources/Cache.ts @@ -7,6 +7,7 @@ import fs from 'fs'; import {Configuration} from './Configuration'; import {MessageName} from './MessageName'; +import {RefCountedCache} from './RefCountedCache'; import {ReportError} from './Report'; import * as hashUtils from './hashUtils'; import * as miscUtils from './miscUtils'; @@ -72,6 +73,8 @@ export class Cache { checksum: string | null, ]>> = new Map(); + private refCountedZipFsCache = new RefCountedCache(zipFs => zipFs.discardAndClose); + /** * To ensure different instances of `Cache` doesn't end up copying to the same * temporary file this random ID is appended to the filename. @@ -467,14 +470,23 @@ export class Cache { if (!shouldMock) this.markedFiles.add(cachePath); - let zipFs: ZipFS | undefined; + let releaseCacheEntry: () => void | undefined; const zipFsBuilder = shouldMock ? () => makeMockPackage() - : () => new ZipFS(cachePath, {baseFs, readOnly: true}); + : () => { + const result = this.refCountedZipFsCache.addOrCreate(cachePath, () => { + return new ZipFS(cachePath, {baseFs, readOnly: true}); + }); + if (releaseCacheEntry !== undefined) + throw new Error(`Race condition in ZipFsBuild & RefCountedZipFsCache`); + + releaseCacheEntry = result.release; + return result.value; + }; const lazyFs = new LazyFS(() => miscUtils.prettifySyncErrors(() => { - return zipFs = zipFsBuilder(); + return zipFsBuilder(); }, message => { return `Failed to open the cache entry for ${structUtils.prettyLocator(this.configuration, locator)}: ${message}`; }), ppath); @@ -484,7 +496,9 @@ export class Cache { const aliasFs = new AliasFS(cachePath, {baseFs: lazyFs, pathUtils: ppath}); const releaseFs = () => { - zipFs?.discardAndClose(); + if (releaseCacheEntry) { + releaseCacheEntry(); + } }; // We hide the checksum if the package presence is conditional, because it becomes unreliable diff --git a/packages/yarnpkg-core/sources/RefCountedCache.ts b/packages/yarnpkg-core/sources/RefCountedCache.ts new file mode 100644 index 000000000000..de888784718e --- /dev/null +++ b/packages/yarnpkg-core/sources/RefCountedCache.ts @@ -0,0 +1,69 @@ +/** + * A cache map with reference counting. This map is designed to handle + * a resource that has native/wasm handles which need to be release explicitly. + * It also requires the value to have a unique map to cache instanches + */ +export class RefCountedCache { + private map: Map; + + /** + * Creates a new RefCountedMap. + * @param releaseFunction The function to release the native resources. + */ + constructor(private releaseFunction: (value: TValue) => void) { + this.map = new Map(); + } + + /** + * + * @param key A unique key to indentify the instance in this Map + * @param createInstance The function to create a new instance of TValue if none already esists + * @returns The value form the cache (or newly created when not present) as well as the release function + * to call when the object is to be released. + */ + addOrCreate(key: TKey, createInstance: () => TValue): {value: TValue, release: () => void} { + const result = this.map.get(key); + if (result) { + // We had an existing value. + if (result.refCount <= 0) + throw new Error(`Race condition in RefCountedMap. While adding a new key the refCount is: ${result.refCount} for ${JSON.stringify(key)}`); + + // Update the refcount. + result.refCount++; + return { + value: result.value, + release: () => this.release(key), + }; + } else { + // we had not seen this before, create the instance and register a refCount of 1. + const newValue = createInstance(); + this.map.set(key, {refCount: 1, value: newValue}); + return { + value: newValue, + release: () => this.release(key), + }; + } + } + + /** + * Releases the object by decreasing the refcount. When the last reference is released (i.e. the refcount goes to 0) + * This function will call to the releaseFunction passed to the cache map to release the native resources. + */ + private release(key: TKey): void { + const result = this.map.get(key); + if (!result) + throw new Error(`Unbalanced calls to release. No known instances of: ${JSON.stringify(key)}`); + + + const refCount = result.refCount; + if (refCount <= 0) { + throw new Error(`Unbalanced calls to release. Too many release vs alloc refcount would become: ${refCount - 1} of ${JSON.stringify(key)}`); + } else if (refCount == 1) { + // This is the last used item. Remove the value from the map and call the releaseFunction to free the native resources + this.map.delete(key); + this.releaseFunction(result.value); + } else { + result.refCount--; + } + } +} diff --git a/packages/yarnpkg-core/tests/RefCountedCache.test.ts b/packages/yarnpkg-core/tests/RefCountedCache.test.ts new file mode 100644 index 000000000000..f079855452fa --- /dev/null +++ b/packages/yarnpkg-core/tests/RefCountedCache.test.ts @@ -0,0 +1,107 @@ +import {RefCountedCache} from '../sources/RefCountedCache'; + +describe(`RefCountedCache`, () => { + it(`should create value on first create`, () => { + const actions: Array = []; + const cache = new RefCountedCache((id => actions.push(`release ${id}`))); + const result = cache.addOrCreate(`a`, () => { + const result = `create a-1`; actions.push(result); return result; + }) ; + expect(result.value).toBe(`create a-1`); + expect(actions).toStrictEqual([`create a-1`]); + }); + + it(`should release single value`, () => { + const actions: Array = []; + const cache = new RefCountedCache((id => actions.push(`release ${id}`))); + const result = cache.addOrCreate(`a`, () => { + const result = `create a-1`; actions.push(result); return result; + }) ; + result.release(); + expect(actions).toStrictEqual([`create a-1`, `release create a-1`]); + }); + + it(`should return first created value and only release on the last value`, () => { + const actions: Array = []; + const cache = new RefCountedCache((id => actions.push(`release ${id}`))); + const result1 = cache.addOrCreate(`a`, () => { + const result = `create a-1`; actions.push(result); return result; + }) ; + expect(result1.value).toBe(`create a-1`); + expect(actions).toStrictEqual([`create a-1`]); + + // Creating new value with same key should reuse the previous value. + const result2 = cache.addOrCreate(`a`, () => { + const result = `create a-2`; actions.push(result); return result; + }) ; + expect(result2.value).toBe(`create a-1`); + expect(actions).toStrictEqual([`create a-1`]); + + // releasing one should not call release function + result1.release(); + expect(actions).toStrictEqual([`create a-1`]); + + // releasing second should call release, but on the first created instance. + result2.release(); + expect(actions).toStrictEqual([`create a-1`, `release create a-1`]); + }); + + it(`should handle multiple keys single value`, () => { + const actions: Array = []; + const cache = new RefCountedCache((id => actions.push(`release ${id}`))); + const result1 = cache.addOrCreate(`a`, () => { + const result = `create a-1`; actions.push(result); return result; + }) ; + result1.release(); + const result2 = cache.addOrCreate(`b`, () => { + const result = `create b-2`; actions.push(result); return result; + }) ; + result2.release(); + const result3 = cache.addOrCreate(`c`, () => { + const result = `create c-3`; actions.push(result); return result; + }) ; + cache.addOrCreate(`d`, () => { + const result = `create d-4`; actions.push(result); return result; + }) ; + const result5 = cache.addOrCreate(`e`, () => { + const result = `create e-5`; actions.push(result); return result; + }) ; + result5.release(); + // skipping release 4 release + result3.release(); + + expect(actions).toStrictEqual([`create a-1`, `release create a-1`, `create b-2`, `release create b-2`, `create c-3`, `create d-4`, `create e-5`, `release create e-5`, `release create c-3`]); + }); + + it(`should can create new instances after removing releasing value`, () => { + const actions: Array = []; + const cache = new RefCountedCache((id => actions.push(`release ${id}`))); + const result1 = cache.addOrCreate(`a`, () => { + const result = `create a-1`; actions.push(result); return result; + }) ; + const result2 = cache.addOrCreate(`a`, () => { + const result = `create a-2`; actions.push(result); return result; + }) ; + result1.release(); + result2.release(); + const result3 = cache.addOrCreate(`a`, () => { + const result = `create a-3`; actions.push(result); return result; + }) ; + const result4 = cache.addOrCreate(`a`, () => { + const result = `create a-4`; actions.push(result); return result; + }) ; + result4.release(); + result3.release(); + expect(actions).toStrictEqual([`create a-1`, `release create a-1`, `create a-3`, `release create a-3`]); + }); + + it(`should throw when releasing too many times`, () => { + const actions: Array = []; + const cache = new RefCountedCache((id => actions.push(`release ${id}`))); + const result1 = cache.addOrCreate(`a`, () => { + const result = `create a-1`; actions.push(result); return result; + }) ; + result1.release(); + expect(result1.release).toThrow(/No known instances of: "a"/); + }); +}); From 171a76da44f6042ff93abf5b4e285f93cdefd241 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ma=C3=ABl=20Nison?= Date: Fri, 28 Mar 2025 10:38:32 +0100 Subject: [PATCH 2/4] Small refactoring --- packages/yarnpkg-core/sources/Cache.ts | 28 +++---- .../yarnpkg-core/sources/RefCountedCache.ts | 31 +++++--- .../tests/RefCountedCache.test.ts | 78 +++++++++++++++---- 3 files changed, 90 insertions(+), 47 deletions(-) diff --git a/packages/yarnpkg-core/sources/Cache.ts b/packages/yarnpkg-core/sources/Cache.ts index 15873c9eb6cb..66d891cd1f8f 100644 --- a/packages/yarnpkg-core/sources/Cache.ts +++ b/packages/yarnpkg-core/sources/Cache.ts @@ -7,7 +7,7 @@ import fs from 'fs'; import {Configuration} from './Configuration'; import {MessageName} from './MessageName'; -import {RefCountedCache} from './RefCountedCache'; +import {RefCountedCache, RefCountedCacheEntry} from './RefCountedCache'; import {ReportError} from './Report'; import * as hashUtils from './hashUtils'; import * as miscUtils from './miscUtils'; @@ -470,23 +470,17 @@ export class Cache { if (!shouldMock) this.markedFiles.add(cachePath); - let releaseCacheEntry: () => void | undefined; + const createRefCount = () => this.refCountedZipFsCache.addOrCreate(cachePath, () => { + return shouldMock + ? makeMockPackage() + : new ZipFS(cachePath, {baseFs, readOnly: true}); + }); - const zipFsBuilder = shouldMock - ? () => makeMockPackage() - : () => { - const result = this.refCountedZipFsCache.addOrCreate(cachePath, () => { - return new ZipFS(cachePath, {baseFs, readOnly: true}); - }); - if (releaseCacheEntry !== undefined) - throw new Error(`Race condition in ZipFsBuild & RefCountedZipFsCache`); - - releaseCacheEntry = result.release; - return result.value; - }; + let refCountedCacheEntry: RefCountedCacheEntry | undefined; const lazyFs = new LazyFS(() => miscUtils.prettifySyncErrors(() => { - return zipFsBuilder(); + refCountedCacheEntry = createRefCount(); + return refCountedCacheEntry.value; }, message => { return `Failed to open the cache entry for ${structUtils.prettyLocator(this.configuration, locator)}: ${message}`; }), ppath); @@ -496,9 +490,7 @@ export class Cache { const aliasFs = new AliasFS(cachePath, {baseFs: lazyFs, pathUtils: ppath}); const releaseFs = () => { - if (releaseCacheEntry) { - releaseCacheEntry(); - } + refCountedCacheEntry?.release(); }; // We hide the checksum if the package presence is conditional, because it becomes unreliable diff --git a/packages/yarnpkg-core/sources/RefCountedCache.ts b/packages/yarnpkg-core/sources/RefCountedCache.ts index de888784718e..aa7c4542b009 100644 --- a/packages/yarnpkg-core/sources/RefCountedCache.ts +++ b/packages/yarnpkg-core/sources/RefCountedCache.ts @@ -1,17 +1,21 @@ +export type RefCountedCacheEntry = { + value: TValue; + release: () => void; +}; + /** * A cache map with reference counting. This map is designed to handle * a resource that has native/wasm handles which need to be release explicitly. * It also requires the value to have a unique map to cache instanches */ export class RefCountedCache { - private map: Map; + private map = new Map(); /** * Creates a new RefCountedMap. * @param releaseFunction The function to release the native resources. */ constructor(private releaseFunction: (value: TValue) => void) { - this.map = new Map(); } /** @@ -21,23 +25,27 @@ export class RefCountedCache { * @returns The value form the cache (or newly created when not present) as well as the release function * to call when the object is to be released. */ - addOrCreate(key: TKey, createInstance: () => TValue): {value: TValue, release: () => void} { + addOrCreate(key: TKey, createInstance: () => TValue): RefCountedCacheEntry { const result = this.map.get(key); - if (result) { - // We had an existing value. + + if (typeof result !== `undefined`) { if (result.refCount <= 0) throw new Error(`Race condition in RefCountedMap. While adding a new key the refCount is: ${result.refCount} for ${JSON.stringify(key)}`); - // Update the refcount. result.refCount++; + return { value: result.value, release: () => this.release(key), }; } else { - // we had not seen this before, create the instance and register a refCount of 1. const newValue = createInstance(); - this.map.set(key, {refCount: 1, value: newValue}); + + this.map.set(key, { + refCount: 1, + value: newValue, + }); + return { value: newValue, release: () => this.release(key), @@ -54,12 +62,11 @@ export class RefCountedCache { if (!result) throw new Error(`Unbalanced calls to release. No known instances of: ${JSON.stringify(key)}`); - const refCount = result.refCount; - if (refCount <= 0) { + if (refCount <= 0) throw new Error(`Unbalanced calls to release. Too many release vs alloc refcount would become: ${refCount - 1} of ${JSON.stringify(key)}`); - } else if (refCount == 1) { - // This is the last used item. Remove the value from the map and call the releaseFunction to free the native resources + + if (refCount == 1) { this.map.delete(key); this.releaseFunction(result.value); } else { diff --git a/packages/yarnpkg-core/tests/RefCountedCache.test.ts b/packages/yarnpkg-core/tests/RefCountedCache.test.ts index f079855452fa..6a9e55a2c21a 100644 --- a/packages/yarnpkg-core/tests/RefCountedCache.test.ts +++ b/packages/yarnpkg-core/tests/RefCountedCache.test.ts @@ -4,9 +4,11 @@ describe(`RefCountedCache`, () => { it(`should create value on first create`, () => { const actions: Array = []; const cache = new RefCountedCache((id => actions.push(`release ${id}`))); + const result = cache.addOrCreate(`a`, () => { const result = `create a-1`; actions.push(result); return result; - }) ; + }); + expect(result.value).toBe(`create a-1`); expect(actions).toStrictEqual([`create a-1`]); }); @@ -14,9 +16,11 @@ describe(`RefCountedCache`, () => { it(`should release single value`, () => { const actions: Array = []; const cache = new RefCountedCache((id => actions.push(`release ${id}`))); + const result = cache.addOrCreate(`a`, () => { const result = `create a-1`; actions.push(result); return result; - }) ; + }); + result.release(); expect(actions).toStrictEqual([`create a-1`, `release create a-1`]); }); @@ -24,16 +28,19 @@ describe(`RefCountedCache`, () => { it(`should return first created value and only release on the last value`, () => { const actions: Array = []; const cache = new RefCountedCache((id => actions.push(`release ${id}`))); + const result1 = cache.addOrCreate(`a`, () => { const result = `create a-1`; actions.push(result); return result; - }) ; + }); + expect(result1.value).toBe(`create a-1`); expect(actions).toStrictEqual([`create a-1`]); // Creating new value with same key should reuse the previous value. const result2 = cache.addOrCreate(`a`, () => { const result = `create a-2`; actions.push(result); return result; - }) ; + }); + expect(result2.value).toBe(`create a-1`); expect(actions).toStrictEqual([`create a-1`]); @@ -49,59 +56,96 @@ describe(`RefCountedCache`, () => { it(`should handle multiple keys single value`, () => { const actions: Array = []; const cache = new RefCountedCache((id => actions.push(`release ${id}`))); + const result1 = cache.addOrCreate(`a`, () => { const result = `create a-1`; actions.push(result); return result; - }) ; + }); + result1.release(); + const result2 = cache.addOrCreate(`b`, () => { const result = `create b-2`; actions.push(result); return result; - }) ; + }); + result2.release(); + const result3 = cache.addOrCreate(`c`, () => { const result = `create c-3`; actions.push(result); return result; - }) ; + }); + cache.addOrCreate(`d`, () => { const result = `create d-4`; actions.push(result); return result; - }) ; + }); + const result5 = cache.addOrCreate(`e`, () => { const result = `create e-5`; actions.push(result); return result; - }) ; + }); + result5.release(); // skipping release 4 release result3.release(); - expect(actions).toStrictEqual([`create a-1`, `release create a-1`, `create b-2`, `release create b-2`, `create c-3`, `create d-4`, `create e-5`, `release create e-5`, `release create c-3`]); + expect(actions).toStrictEqual([ + `create a-1`, + `release create a-1`, + `create b-2`, + `release create b-2`, + `create c-3`, + `create d-4`, + `create e-5`, + `release create e-5`, + `release create c-3`, + `release create d-4`, + ]); }); it(`should can create new instances after removing releasing value`, () => { const actions: Array = []; const cache = new RefCountedCache((id => actions.push(`release ${id}`))); + const result1 = cache.addOrCreate(`a`, () => { const result = `create a-1`; actions.push(result); return result; - }) ; + }); + const result2 = cache.addOrCreate(`a`, () => { const result = `create a-2`; actions.push(result); return result; - }) ; + }); + result1.release(); result2.release(); + const result3 = cache.addOrCreate(`a`, () => { const result = `create a-3`; actions.push(result); return result; - }) ; + }); + const result4 = cache.addOrCreate(`a`, () => { const result = `create a-4`; actions.push(result); return result; - }) ; + }); + result4.release(); result3.release(); - expect(actions).toStrictEqual([`create a-1`, `release create a-1`, `create a-3`, `release create a-3`]); + + expect(actions).toStrictEqual([ + `create a-1`, + `release create a-1`, + `create a-3`, + `release create a-3`, + `create a-4`, + ]); }); it(`should throw when releasing too many times`, () => { const actions: Array = []; const cache = new RefCountedCache((id => actions.push(`release ${id}`))); + const result1 = cache.addOrCreate(`a`, () => { const result = `create a-1`; actions.push(result); return result; - }) ; + }); + result1.release(); - expect(result1.release).toThrow(/No known instances of: "a"/); + + expect(() => { + result1.release(); + }).toThrow(/No known instances of: "a"/); }); }); From 306242106f5ea553ae7074a7fd1aa0bbd89cecf5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ma=C3=ABl=20Nison?= Date: Fri, 28 Mar 2025 10:53:11 +0100 Subject: [PATCH 3/4] Remove redundant test case creation --- packages/yarnpkg-core/tests/RefCountedCache.test.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/yarnpkg-core/tests/RefCountedCache.test.ts b/packages/yarnpkg-core/tests/RefCountedCache.test.ts index 6a9e55a2c21a..670a89342124 100644 --- a/packages/yarnpkg-core/tests/RefCountedCache.test.ts +++ b/packages/yarnpkg-core/tests/RefCountedCache.test.ts @@ -130,7 +130,6 @@ describe(`RefCountedCache`, () => { `release create a-1`, `create a-3`, `release create a-3`, - `create a-4`, ]); }); From 3f783bac7b320ade1314a083b13de3a95f96e9b2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ma=C3=ABl=20Nison?= Date: Fri, 28 Mar 2025 11:02:26 +0100 Subject: [PATCH 4/4] Bad Cursor, bad --- packages/yarnpkg-core/tests/RefCountedCache.test.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/yarnpkg-core/tests/RefCountedCache.test.ts b/packages/yarnpkg-core/tests/RefCountedCache.test.ts index 670a89342124..3d4cc23d29fe 100644 --- a/packages/yarnpkg-core/tests/RefCountedCache.test.ts +++ b/packages/yarnpkg-core/tests/RefCountedCache.test.ts @@ -95,7 +95,6 @@ describe(`RefCountedCache`, () => { `create e-5`, `release create e-5`, `release create c-3`, - `release create d-4`, ]); });