From 963a2bd671f4f65bcdb99b125a0aed228740fad2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nicol=C3=B2=20Ribaudo?= Date: Wed, 2 Oct 2019 23:58:38 +0200 Subject: [PATCH] Don't run cached function two times concurrently if it could be cached --- packages/babel-core/src/config/caching.js | 190 +++++++++++++++--- .../src/config/files/configuration.js | 1 - packages/babel-core/src/config/files/utils.js | 3 +- packages/babel-core/src/config/full.js | 4 +- .../babel-core/src/gensync-utils/async.js | 6 + packages/babel-core/test/caching-api.js | 62 +++++- 6 files changed, 233 insertions(+), 33 deletions(-) diff --git a/packages/babel-core/src/config/caching.js b/packages/babel-core/src/config/caching.js index 0dd0b5d6c117..9886fba08949 100644 --- a/packages/babel-core/src/config/caching.js +++ b/packages/babel-core/src/config/caching.js @@ -1,7 +1,14 @@ // @flow +/*:: declare var invariant; */ + import gensync, { type Handler } from "gensync"; -import { maybeAsync, isAsync, isThenable } from "../gensync-utils/async"; +import { + maybeAsync, + isAsync, + waitFor, + isThenable, +} from "../gensync-utils/async"; import { isIterableIterator } from "./util"; export type { CacheConfigurator }; @@ -39,61 +46,68 @@ function* genTrue(data: any) { export function makeWeakCache( handler: (ArgT, CacheConfigurator) => Handler | ResultT, - invalidateAsync?: boolean = false, ): (ArgT, SideChannel) => Handler { - return makeCachedFunction( - WeakMap, - invalidateAsync, - handler, - ); + return makeCachedFunction(WeakMap, handler); } export function makeWeakCacheSync( handler: (ArgT, CacheConfigurator) => ResultT, ): (ArgT, SideChannel) => ResultT { return synchronize<[ArgT, SideChannel], ResultT>( - makeWeakCache(handler, false), + makeWeakCache(handler), ); } export function makeStrongCache( handler: (ArgT, CacheConfigurator) => Handler | ResultT, - invalidateAsync?: boolean = false, ): (ArgT, SideChannel) => Handler { - return makeCachedFunction( - Map, - invalidateAsync, - handler, - ); + return makeCachedFunction(Map, handler); } export function makeStrongCacheSync( handler: (ArgT, CacheConfigurator) => ResultT, ): (ArgT, SideChannel) => ResultT { return synchronize<[ArgT, SideChannel], ResultT>( - makeStrongCache(handler, false), + makeStrongCache(handler), ); } function makeCachedFunction( CallCache: Class, - invalidateAsync: boolean, handler: (ArgT, CacheConfigurator) => Handler | ResultT, ): (ArgT, SideChannel) => Handler { const callCacheSync = new CallCache(); - const callCacheAsync = invalidateAsync ? new CallCache() : callCacheSync; + const callCacheAsync = new CallCache(); + const futureCache = new CallCache(); + const cacheNotConfigured = new CallCache(); return function* cachedFunction(arg: ArgT, data: SideChannel) { - const callCache = - invalidateAsync && (yield* isAsync()) ? callCacheAsync : callCacheSync; + const asyncContext = yield* isAsync(); + const callCache = asyncContext ? callCacheAsync : callCacheSync; + + const cached = yield* getCachedValueOrWait( + asyncContext, + callCache, + futureCache, + cacheNotConfigured, + arg, + data, + ); - const cached = yield* getCachedValue(callCache, arg, data); - if (cached.valid) { - return cached.value; + switch (cached.kind) { + case "cached": + return cached.value; + case "retry": + return yield* cachedFunction(arg, data); + case "continue": // continue } const cache = new CacheConfigurator(data); + const finishLock: false | Lock = + asyncContext && + setupAsyncLocks(cache, futureCache, cacheNotConfigured, arg); + const handlerResult = handler(arg, cache); const gen = isIterableIterator(handlerResult); @@ -103,6 +117,12 @@ function makeCachedFunction( updateFunctionCache(callCache, cache, arg, value); + if (asyncContext) { + /*:: invariant(finishLock) */ + futureCache.delete(arg); + finishLock.release(value); + } + return value; }; } @@ -133,6 +153,91 @@ function* getCachedValue< return { valid: false, value: null }; } +/* NOTE: This comment refers to both the getCachedValueOrWait and setupAsyncLocks functions. + * + * > There are only two hard things in Computer Science: cache invalidation and naming things. + * > -- Phil Karlton + * + * I don't know if Phil was also thinking about handling a cache whose invalidation function is + * defined asynchronously is considered, but it is REALLY hard to do correctly. + * + * The implemented logic (only when gensync is run asynchronously) is the following: + * 1. If there is a valid cache associated to the current "arg" parameter, + * a. RETURN the cached value + * 2. If there is a ConfigLock associated to the current "arg" parameter, + * a. Wait for that lock to be released + * b. GOTO step 1 + * 3. If there is a FinishLock associated to the current "arg" parameter representing a valid cache, + * a. Wait for that lock to be released + * b. RETURN the value associated with that lock + * 4. Acquire to locks: ConfigLock and FinishLock + * 5. Start executing the cached function + * 6. When the cache if configured, release ConfigLock + * 7. Wait for the function to finish executing + * 8. Release FinishLock + * 9. Send the function result to anyone waiting on FinishLock + * 10. Store the result in the cache + * 11. RETURN the result + */ +function* getCachedValueOrWait( + asyncContext: boolean, + callCache: CacheMap, + futureCache: CacheMap, SideChannel>, + cacheNotConfigured: Map>, + arg: ArgT, + data: SideChannel, +): Handler< + | { kind: "cached", value: ResultT } + | { kind: "retry" | "continue", value: null }, +> { + const cached = yield* getCachedValue(callCache, arg, data); + if (cached.valid) { + return { kind: "cached", value: cached.value }; + } + + if (asyncContext) { + const lock = cacheNotConfigured.get(arg); + if (lock && !lock.released) { + yield* waitFor(lock.promise); + return { kind: "retry", value: null }; + } + + const cached = yield* getCachedValue(futureCache, arg, data); + if (cached.valid) { + const value = yield* waitFor(cached.value.promise); + return { kind: "cached", value: value }; + } + } + + return { kind: "continue", value: null }; +} + +function setupAsyncLocks( + config: CacheConfigurator, + futureCache: CacheMap, SideChannel>, + cacheNotConfigured: Map>, + arg: ArgT, +): Lock { + const finishLock = new Lock(); + + const configLock = new Lock(); + config.onConfigured(() => { + cacheNotConfigured.delete(arg); + configLock.release(); + + updateFunctionCache( + futureCache, + config, + arg, + finishLock, + /* deactivate */ false, + ); + }); + cacheNotConfigured.set(arg, configLock); + + return finishLock; +} + function updateFunctionCache< ArgT, ResultT, @@ -144,12 +249,13 @@ function updateFunctionCache< config: CacheConfigurator, arg: ArgT, value: ResultT, + deactivate?: boolean = true, ) { if (!config.configured()) config.forever(); let cachedValue: CacheEntry | void = cache.get(arg); - config.deactivate(); + if (deactivate) config.deactivate(); switch (config.mode()) { case "forever": @@ -177,6 +283,7 @@ class CacheConfigurator { _invalidate: boolean = false; _configured: boolean = false; + _configuredHandler: ?Function = null; _pairs: Array<[mixed, (SideChannel) => Handler]> = []; @@ -205,7 +312,7 @@ class CacheConfigurator { throw new Error("Caching has already been configured with .never()"); } this._forever = true; - this._configured = true; + this._fireConfigured(); } never() { @@ -216,7 +323,7 @@ class CacheConfigurator { throw new Error("Caching has already been configured with .forever()"); } this._never = true; - this._configured = true; + this._fireConfigured(); } using(handler: SideChannel => T): T { @@ -228,7 +335,7 @@ class CacheConfigurator { "Caching has already been configured with .never or .forever()", ); } - this._configured = true; + this._fireConfigured(); const key = handler(this._data); @@ -270,6 +377,20 @@ class CacheConfigurator { configured() { return this._configured; } + + onConfigured(cb: Function) { + if (this._configuredHandler) { + throw new Error( + "An handler for the 'configured' event of the cache has already been registered", + ); + } + this._configuredHandler = cb; + } + + _fireConfigured() { + this._configured = true; + if (this._configuredHandler) this._configuredHandler(); + } } function makeSimpleConfigurator( @@ -318,3 +439,20 @@ export function assertSimpleType(value: mixed): SimpleType { } return value; } + +class Lock { + released: boolean = false; + promise: Promise; + _resolve: (value: T) => void; + + constructor() { + this.promise = new Promise(resolve => { + this._resolve = resolve; + }); + } + + release(value: T) { + this.released = true; + this._resolve(value); + } +} diff --git a/packages/babel-core/src/config/files/configuration.js b/packages/babel-core/src/config/files/configuration.js index f2541f155208..ec61f1797d19 100644 --- a/packages/babel-core/src/config/files/configuration.js +++ b/packages/babel-core/src/config/files/configuration.js @@ -206,7 +206,6 @@ const readConfigJS = makeStrongCache(function* readConfigJS( } if (typeof options.then === "function") { - // When removing this error, ENABLE the invalidateAsync param of makeStrongCache throw new Error( `You appear to be using an async configuration, ` + `which your current version of Babel does not support. ` + diff --git a/packages/babel-core/src/config/files/utils.js b/packages/babel-core/src/config/files/utils.js index 7e08edfc53d4..3b070ebb51e1 100644 --- a/packages/babel-core/src/config/files/utils.js +++ b/packages/babel-core/src/config/files/utils.js @@ -23,8 +23,7 @@ export function makeStaticFileCache( } return fn(filepath, yield* fs.readFile(filepath, "utf8")); - }, - /* invalidateAsync */ true): Gensync); + }): Gensync); } function* fileMtime(filepath: string): Handler { diff --git a/packages/babel-core/src/config/full.js b/packages/babel-core/src/config/full.js index 07dd09c656e4..ab24a1936b87 100644 --- a/packages/babel-core/src/config/full.js +++ b/packages/babel-core/src/config/full.js @@ -210,7 +210,6 @@ const loadDescriptor = makeWeakCache(function*( if (typeof item.then === "function") { yield* []; // if we want to support async plugins - // When enabling this, ENABLE the invalidateAsync param of makeStrongCache throw new Error( `You appear to be using an async plugin, ` + `which your current version of Babel does not support. ` + @@ -287,8 +286,7 @@ const instantiatePlugin = makeWeakCache(function*( } return new Plugin(plugin, options, alias); -}, -/* invalidateAsync */ true); +}); const validateIfOptionNeedsFilename = ( options: ValidatedOptions, diff --git a/packages/babel-core/src/gensync-utils/async.js b/packages/babel-core/src/gensync-utils/async.js index 3aa80008e3a0..b4d9eb3c30b6 100644 --- a/packages/babel-core/src/gensync-utils/async.js +++ b/packages/babel-core/src/gensync-utils/async.js @@ -64,6 +64,12 @@ export function forwardAsync( }); } +const id = x => x; +export const waitFor = (gensync<[any], any>({ + sync: id, + async: id, +}): (p: T | Promise) => Handler); + export function isThenable(val: mixed): boolean %checks { return ( /*:: val instanceof Promise && */ diff --git a/packages/babel-core/test/caching-api.js b/packages/babel-core/test/caching-api.js index 8bdfee9030e7..c5d151d78ba7 100644 --- a/packages/babel-core/test/caching-api.js +++ b/packages/babel-core/test/caching-api.js @@ -1,4 +1,5 @@ -import { makeStrongCacheSync } from "../lib/config/caching"; +import gensync from "gensync"; +import { makeStrongCacheSync, makeStrongCache } from "../lib/config/caching"; describe("caching API", () => { it("should allow permacaching with .forever()", () => { @@ -410,4 +411,63 @@ describe("caching API", () => { expect(fn("two")).toBe(fn("two")); }); }); + + describe.only("async", () => { + const wait = gensync({ + sync: () => {}, + errback: (t, cb) => setTimeout(cb, t), + }); + + it("should use a cache even if configured late", async () => { + const fn = gensync( + makeStrongCache(function*(arg, cache) { + yield* wait(50); + cache.forever(); + return { arg }; + }), + ).async; + + const [res1, res2] = await Promise.all([fn("bar"), fn("bar")]); + + expect(res1).toBe(res2); + }); + + it("should invalidate a cache even if configured late", async () => { + const fn = gensync( + makeStrongCache(function*(arg, cache) { + yield* wait(50); + cache.never(); + return { arg }; + }), + ).async; + + const [res1, res2] = await Promise.all([fn("bar"), fn("bar")]); + + expect(res1).not.toBe(res2); + expect(res1).toEqual(res2); + }); + + it("should not wait for configuration if the parameter is different", async () => { + let configured = 0; + + const fn = gensync( + makeStrongCache(function*(arg, cache) { + configured++; + yield* wait(50); + cache.forever(); + return { arg }; + }), + ).async; + + const promises = [fn("bar"), fn("foo")]; + + await wait.async(10); + + expect(configured).toBe(2); + + await Promise.all(promises); + + expect(configured).toBe(2); + }); + }); });