diff --git a/src/storage/conversion/ChainedConverter.ts b/src/storage/conversion/ChainedConverter.ts index b99b1c94d5..defa8dc21a 100644 --- a/src/storage/conversion/ChainedConverter.ts +++ b/src/storage/conversion/ChainedConverter.ts @@ -31,67 +31,6 @@ type MatchedPath = { weight: number; }; -/** - * An LRU cache for storing `ConversionPath`s. - */ -class LruPathCache { - private readonly maxSize: number; - // Contents are ordered from least to most recently used - private readonly paths: ConversionPath[] = []; - - public constructor(maxSize: number) { - this.maxSize = maxSize; - } - - /** - * Add the given path to the cache as most recently used. - */ - public add(path: ConversionPath): void { - this.paths.push(path); - if (this.paths.length > this.maxSize) { - this.paths.shift(); - } - } - - /** - * Find a path that can convert the given type to the given preferences. - * Note that this finds the first matching path in the cache, - * not the best one, should there be multiple results. - * In practice this should almost never be the case though. - */ - public find(inType: string, outPreferences: ValuePreferences): MatchedPath | undefined { - // Last element is most recently used so has more chance of being the correct one - for (let i = this.paths.length - 1; i >= 0; --i) { - const path = this.paths[i]; - // Check if `inType` matches the input and `outPreferences` the output types of the path - const match = this.getMatchedPath(inType, outPreferences, path); - if (match) { - // Set matched path to most recent result in the cache - this.paths.splice(i, 1); - this.paths.push(path); - return match; - } - } - } - - /** - * Calculates the weights and exact types when using the given path on the given type and preferences. - * Undefined if there is no match - */ - private getMatchedPath(inType: string, outPreferences: ValuePreferences, path: ConversionPath): - MatchedPath | undefined { - const inWeight = getTypeWeight(inType, path.inTypes); - if (inWeight === 0) { - return; - } - const outMatch = getBestPreference(path.outTypes, outPreferences); - if (!outMatch) { - return; - } - return { path, inType, outType: outMatch.value, weight: inWeight * outMatch.weight }; - } -} - /** * A meta converter that takes an array of other converters as input. * It chains these converters by finding a path of converters @@ -99,39 +38,36 @@ class LruPathCache { * In case there are multiple paths, the one with the highest weight gets found. * Will error in case no path can be found. * - * Generated paths get stored in an internal cache for later re-use on similar requests. - * Note that due to this caching `RepresentationConverter`s - * that change supported input/output types at runtime are not supported, - * unless cache size is set to 0. - * * This is not a TypedRepresentationConverter since the supported output types * might depend on what is the input content-type. * + * This converter should be the last in a WaterfallHandler if there are multiple, + * since it will try to convert any representation with a content-type. + * * Some suggestions on how this class can be even more optimized should this ever be needed in the future. * Most of these decrease computation time at the cost of more memory. - * - Subpaths that are generated could also be cached. - * - When looking for the next step, cached paths could also be considered. * - The algorithm could start on both ends of a possible path and work towards the middle. * - When creating a path, store the list of unused converters instead of checking every step. + * - Caching: https://github.com/solid/community-server/issues/832 */ export class ChainedConverter extends RepresentationConverter { protected readonly logger = getLoggerFor(this); private readonly converters: TypedRepresentationConverter[]; - private readonly cache: LruPathCache; - public constructor(converters: TypedRepresentationConverter[], maxCacheSize = 50) { + public constructor(converters: TypedRepresentationConverter[]) { super(); if (converters.length === 0) { throw new Error('At least 1 converter is required.'); } this.converters = [ ...converters ]; - this.cache = new LruPathCache(maxCacheSize); } public async canHandle(input: RepresentationConverterArgs): Promise { - // Will cache the path if found, and error if not - await this.findPath(input); + const type = input.representation.metadata.contentType; + if (!type) { + throw new BadRequestHttpError('Missing Content-Type header.'); + } } public async handle(input: RepresentationConverterArgs): Promise { @@ -156,24 +92,15 @@ export class ChainedConverter extends RepresentationConverter { return path.converters.slice(-1)[0].handle(args); } - public async handleSafe(input: RepresentationConverterArgs): Promise { - // This way we don't run `findPath` twice, even though it would be cached for the second call - return this.handle(input); - } - private isMatchedPath(path: unknown): path is MatchedPath { return typeof (path as MatchedPath).path === 'object'; } /** - * Finds a conversion path that can handle the given input, - * either in the cache or by generating a new one. + * Finds a conversion path that can handle the given input. */ private async findPath(input: RepresentationConverterArgs): Promise { - const type = input.representation.metadata.contentType; - if (!type) { - throw new BadRequestHttpError('Missing Content-Type header.'); - } + const type = input.representation.metadata.contentType!; const preferences = cleanPreferences(input.preferences.type); const weight = getTypeWeight(type, preferences); @@ -182,15 +109,7 @@ export class ChainedConverter extends RepresentationConverter { return { value: type, weight }; } - // Use a cached solution if we have one. - // Note that it's possible that a better one could be generated. - // But this is usually highly unlikely. - let match = this.cache.find(type, preferences); - if (!match) { - match = await this.generatePath(type, preferences); - this.cache.add(match.path); - } - return match; + return this.generatePath(type, preferences); } /** diff --git a/test/unit/storage/conversion/ChainedConverter.test.ts b/test/unit/storage/conversion/ChainedConverter.test.ts index e4879fb013..6afc73b330 100644 --- a/test/unit/storage/conversion/ChainedConverter.test.ts +++ b/test/unit/storage/conversion/ChainedConverter.test.ts @@ -77,7 +77,7 @@ describe('A ChainedConverter', (): void => { const converter = new ChainedConverter(converters); args.representation.metadata.contentType = 'b/b'; - await expect(converter.canHandle(args)).rejects + await expect(converter.handle(args)).rejects .toThrow('No conversion path could be made from b/b to x/x,x/*,internal/*.'); }); @@ -218,104 +218,4 @@ describe('A ChainedConverter', (): void => { expect(converter.handle).toHaveBeenCalledTimes(1); expect(converter.handle).toHaveBeenLastCalledWith(args); }); - - it('caches paths for re-use.', async(): Promise => { - const converters = [ - new DummyConverter({ 'a/a': 0.8 }, { 'b/b': 0.9 }), - new DummyConverter({ 'b/b': 0.8 }, { 'x/x': 1 }), - ]; - const converter = new ChainedConverter(converters); - let result = await converter.handle(args); - expect(result.metadata.contentType).toBe('x/x'); - - jest.spyOn(converters[0], 'getInputTypes'); - jest.spyOn(converters[0], 'getOutputTypes'); - result = await converter.handle(args); - expect(result.metadata.contentType).toBe('x/x'); - expect(converters[0].getInputTypes).toHaveBeenCalledTimes(0); - expect(converters[0].getOutputTypes).toHaveBeenCalledTimes(0); - }); - - it('removes unused paths from the cache.', async(): Promise => { - const converters = [ - new DummyConverter({ 'a/a': 0.8 }, { 'b/b': 0.9 }), - new DummyConverter({ 'b/b': 0.8 }, { 'x/x': 1 }), - new DummyConverter({ 'c/c': 0.8 }, { 'b/b': 0.9 }), - ]; - // Cache size 1 - const converter = new ChainedConverter(converters, 1); - let result = await converter.handle(args); - expect(result.metadata.contentType).toBe('x/x'); - - // Should remove previous path (which contains converter 0) - args.representation.metadata.contentType = 'c/c'; - result = await converter.handle(args); - expect(result.metadata.contentType).toBe('x/x'); - - jest.spyOn(converters[0], 'getInputTypes'); - jest.spyOn(converters[0], 'getOutputTypes'); - args.representation.metadata.contentType = 'a/a'; - result = await converter.handle(args); - expect(result.metadata.contentType).toBe('x/x'); - expect(converters[0].getInputTypes).not.toHaveBeenCalledTimes(0); - expect(converters[0].getOutputTypes).not.toHaveBeenCalledTimes(0); - }); - - it('keeps the most recently used paths in the cache.', async(): Promise => { - const converters = [ - new DummyConverter({ 'a/a': 1 }, { 'd/d': 1 }), - new DummyConverter({ 'b/b': 1 }, { 'd/d': 1 }), - new DummyConverter({ 'c/c': 1 }, { 'd/d': 1 }), - new DummyConverter({ 'd/d': 1 }, { 'x/x': 1 }), - ]; - // Cache size 2 - const converter = new ChainedConverter(converters, 2); - // Caches path 0 - await converter.handle(args); - - // Caches path 1 - args.representation.metadata.contentType = 'b/b'; - await converter.handle(args); - - // Reset path 0 in cache - args.representation.metadata.contentType = 'a/a'; - await converter.handle(args); - - // Caches path 2 and removes 1 - args.representation.metadata.contentType = 'c/c'; - await converter.handle(args); - - jest.spyOn(converters[0], 'getInputTypes'); - jest.spyOn(converters[1], 'getInputTypes'); - jest.spyOn(converters[2], 'getInputTypes'); - - // Path 0 and 2 should be cached now - args.representation.metadata.contentType = 'a/a'; - await converter.handle(args); - expect(converters[0].getInputTypes).toHaveBeenCalledTimes(0); - args.representation.metadata.contentType = 'c/c'; - await converter.handle(args); - expect(converters[2].getInputTypes).toHaveBeenCalledTimes(0); - args.representation.metadata.contentType = 'b/b'; - await converter.handle(args); - expect(converters[1].getInputTypes).not.toHaveBeenCalledTimes(0); - }); - - it('does not use cached paths that match content-type but not preferences.', async(): Promise => { - const converters = [ - new DummyConverter({ 'a/a': 1 }, { 'b/b': 1 }), - new DummyConverter({ 'b/b': 1 }, { 'x/x': 1 }), - new DummyConverter({ 'a/a': 1 }, { 'c/c': 1 }), - new DummyConverter({ 'c/c': 1 }, { 'y/y': 1 }), - ]; - const converter = new ChainedConverter(converters); - - // Cache a-b-x path - await converter.handle(args); - - // Generate new a-c-y path - args.preferences.type = { 'y/y': 1 }; - const result = await converter.handle(args); - expect(result.metadata.contentType).toBe('y/y'); - }); });