Plugin cache #2389

guybedford · 2018-08-08T12:07:11Z

This provides a plugin cache on plugin contexts with the API:

class PluginCache {
  get (key: string): any;
  set (key: string, value: any): void;
  has (key: string): boolean;
  delete (key: string): boolean;
}

The cache is stored in bundle.cache.plugins[cacheKey] as a plain object, which is not directly accessible.

Each value in the cache is stored as [number, value], where the first number indicates the access count. This is zero if it was accessed in the last build, and is incremented each build. When it reaches 10 (10 builds without any access through get, set, or has) we automatically evict the cache item.

The cache key for a plugin is plugin.cacheKey || plugin.name. When using plugin.name this will throw if on duplicate or missing plugin names as soon as cache access is attempted (a get / set / has / delete call).

Two instances of the same plugin can support sharing a cache instance in the same build provided they use a plugin.cacheKey and not a plugin.name.

There are some complexities around supporting the transform caching and transform dependencies stuff. We basically detect this.cache usage in transform and then fully opt-out of the automatic transform caching - no transform dependencies invalidations, no asset reemissions etc.

The hope is to eventually deprecate the automatic transform caching along with its invalidations and asset reemissions. To prepare for this watchFiles has been refactored in the build as well, with the goal to just always rerunning all hooks and relying on the plugin internal cache.

Edit - New additions:

cacheExpires option: allows setting an integer representing how many builds should run before unused keys are cleared from the plugin cache
this.addWatchFile in plugins - allows a more generic feature for plugin watcher dependencies, as previously tracked in Consider supporting transformDependencies for load #2363. Again this aligns with moving away from special transform handling and deprecating both transform dependencies and auto caching.

keithamus · 2018-08-08T12:16:53Z

src/utils/pluginDriver.ts


 	let hasLoadersOrTransforms = false;

 	const pluginContexts = plugins.map(plugin => {
+		let cacheable = true;
+		if (typeof plugin.name === 'string') {
+			if (existingPluginKeys.indexOf(plugin.name) === -1) cacheable = false;


Is this correct? Do you mean !== -1 here? I'm a little confused at this block of code.

Yup, did not mean to imply this was bug-free just yet :)

keithamus · 2018-08-08T12:21:29Z

src/utils/pluginDriver.ts

+		if (typeof plugin.name === 'string') {
+			if (existingPluginKeys.indexOf(plugin.name) !== -1) cacheable = false;
+			existingPluginKeys.push(plugin.name);
+		}


This block might be better to start with cacheable = false, then setting it to true if it is unique. Currently, if I'm following the logic correctly, plugins with no name will be left in a state of cachable = true

Yes, this was half-baked and missed the logic I intended wrt cacheKey allowing shared instance caches between multiple instances of the same plugin in the build.

shellscape · 2018-08-08T13:20:29Z

This is shaping up nicely 👍

guybedford · 2018-08-10T10:51:56Z

Ok the tests are up, ready for review.

keithamus · 2018-08-10T10:56:28Z

src/utils/pluginDriver.ts


 	let hasLoadersOrTransforms = false;

 	const pluginContexts = plugins.map(plugin => {
+		let cacheable = true;
+		if (typeof plugin.cacheKey === 'string') {


This is an empty condition. Is it necessary?

It's a ruling-out case for the others to avoid a repeated check. Could be done better certainly, but that was the logic that made sense to me.

I've "conventionalized" it :P

guybedford · 2018-08-10T11:10:45Z

I've updated the description in the PR now as well so it is clear what is being provided here.

keithamus · 2018-08-10T12:28:53Z

src/Graph.ts

@@ -34,6 +36,9 @@ import relativeId, { getAliasName } from './utils/relativeId';
 import { timeEnd, timeStart } from './utils/timers';
 import transform from './utils/transform';

+// clear plugin cache items after 10 builds unnaccessed
+const PLUGIN_CACHE_EVICTION_EXPIRY = 10;


It would be good to see this number configurable; CI machines might build dozens of branches per day may benefit from larger numbers so they aren't evicting cache quite as much.

@keithamus would that be better as an environment variable, or configuration option?

I guess a configuration option - considering pretty much everything else is. With a rollup.config.js it's relatively easy to expose it as a environment variable.

I've added a cacheExpiry configuration option.

lukastaegert

Looks good! The only thing that bothers me somewhat is the cache expiry: What use should this have?

If we want to prevent invalid caches by resetting the cache after n runs, why would there be no invalid cache after n-1 runs? Or is the idea that if the watcher does not trigger, people should just keep making changes until they hit the expiry limit?

I would rather have we double down on making sure that the cache is always cleared in all necessary situations. Otherwise this will also mask errors in the caching that people will probably not report to us because they consider them to be "slip-ups" while making it slower for everyone.

lukastaegert · 2018-08-11T06:36:31Z

src/rollup/types.d.ts

@@ -353,9 +364,14 @@ export interface OutputChunk {
 	map?: SourceMap;
 }

+export interface SerialisablePluginCache {


Talking about rebelliously European spelling...

Have to double down on that stuff :P

Ah serialize is British English - will change.

lukastaegert · 2018-08-11T06:45:08Z

src/utils/assetHooks.ts

@@ -96,6 +97,11 @@ export function createAssetPluginHooks(
 			const asset: Asset = { name, source, fileName: undefined, dependencies, transform: null };
 			if (outputBundle && source !== undefined) finaliseAsset(asset, outputBundle, assetFileNames);
 			assetsById.set(assetId, asset);
+			if (!asset.transform && asset.dependencies && asset.dependencies.length) {
+				for (const depId of asset.dependencies) {
+					if (watchFiles.indexOf(depId) === -1) watchFiles.push(depId);


Maybe use an object to store the names of watched files for slightly better performance? But probably only a micro optimization

This would also provide automatic deduplication, though.

Agreed, added.

lukastaegert · 2018-08-11T06:50:14Z

src/utils/pluginDriver.ts

+			cache[id] = [0, value];
+		},
+		delete(id: string) {
+			return delete cache[id];


As we only check for thruthiness in .has, maybe overwrite deleted keys with null instead of actually deleting to avoid lots of hidden class transformations and deoptimizations in the engine?

Objects with lots of key changes are treated as "dictionary objects" with hash table keys (which is exactly what we need here). In that case the delete cost is the delete cost of the hash table which shouldn't be a problem. Also we do want to evict unused keys to avoid cache bloat.

guybedford · 2018-08-11T09:41:49Z

If we want to prevent invalid caches by resetting the cache after n runs, why would there be no invalid cache after n-1 runs? Or is the idea that if the watcher does not trigger, people should just keep making changes until they hit the expiry limit?

The cache expiry is used to avoid memory bloat, not to avoid false positives. That is, when we persist the cache to disk, then we need to ensure that the size of the cache is not going to be monotonic. So every time an entry is touched we bump its counter, and if it hasn't been touched for 10 builds, we evict it to handle this case.

Definitely worth making sure this is properly documented though, given the ease of confusion here.

guybedford · 2018-08-11T09:51:28Z

Included feedback, and rebased to plugin-driver.

lukastaegert · 2018-08-11T16:26:13Z

Definitely worth making sure this is properly documented though, given the ease of confusion here

Yes indeed. In that case, I would strongly suggest to make this an experimental option for now to signal we might still change how this is configured. Does not feel like something people should play around with usually and in case there are reasons to change it, maybe there is a better way to configure it than with a number but we do not know yet

tivac · 2018-08-12T02:55:33Z

How will using this.addWatchFile work in practice? Currently with transform dependencies if a dependency changes the build will restart and transform will be called for the module that registered that dependency. If that approach is now deprecated I'd like to better understand the replacement since I'll need to port over to it and the previous behavior, while not ideal, eventually worked for my needs.

lukastaegert · 2018-08-12T08:54:30Z

Sorry, forgot to change the base of this branch before merge.

…ations

… refactoring

guybedford · 2018-08-12T12:03:44Z

Just rebasing, will merge shortly.

guybedford · 2018-08-12T12:30:37Z

@tivac

How will using this.addWatchFile work in practice? Currently with transform dependencies if a dependency changes the build will restart and transform will be called for the module that registered that dependency. If that approach is now deprecated I'd like to better understand the replacement since I'll need to port over to it and the previous behavior, while not ideal, eventually worked for my needs.

So the idea is to deprecate both the current transform dependencies and transform cache, eventually, but the deprecation path hasn't even started yet so no need to worry here things will continue to work fine even in 1.0 most likely for a 2.0 removal.

The new approach is:

addWatchFile adds a file for the watcher to invalidate the build on. This invalidation does not affect the build caching at all though, unlike currently where the transform dependencies invalidate the transform cache.
Instead of the transform hook by default being cached, all hooks will always be re-run with cache information at this.cache. The plugin has its own responsibility to emit assets and check this.cache to handle cached running.
The full separation between addWatchFile and transform cache will provide more flexible fine-grained cache behaviours.

keithamus reviewed Aug 8, 2018

View reviewed changes

guybedford force-pushed the plugin-cache branch from 508e2c2 to 601ef37 Compare August 8, 2018 12:18

keithamus reviewed Aug 8, 2018

View reviewed changes

guybedford force-pushed the plugin-cache branch 2 times, most recently from 67ae9d3 to fad9921 Compare August 8, 2018 12:54

guybedford mentioned this pull request Aug 9, 2018

Fix #337 Keep isCjsPromises between commonjs() calls rollup/rollup-plugin-commonjs#338

Merged

guybedford force-pushed the plugin-cache branch from 635ecc1 to b6c531f Compare August 10, 2018 10:51

guybedford changed the title ~~WIP: plugin cache~~ Plugin cache Aug 10, 2018

guybedford requested a review from lukastaegert August 10, 2018 10:52

guybedford force-pushed the plugin-cache branch from b6c531f to dc2b2a6 Compare August 10, 2018 10:52

keithamus reviewed Aug 10, 2018

View reviewed changes

guybedford force-pushed the plugin-cache branch from 874a2b4 to c3de91b Compare August 10, 2018 15:29

guybedford mentioned this pull request Aug 10, 2018

Consider supporting transformDependencies for load #2363

Closed

lukastaegert reviewed Aug 11, 2018

View reviewed changes

guybedford force-pushed the plugin-cache branch from 53862cf to 8ecb1e0 Compare August 11, 2018 09:51

lukastaegert closed this Aug 12, 2018

lukastaegert changed the base branch from plugin-driver to master August 12, 2018 08:54

lukastaegert reopened this Aug 12, 2018

guybedford added 3 commits August 12, 2018 14:00

asset fixes and refactoring for caching

8432cb5

revert cacheType

a8a9194

refactor plugin hooks behind a single interface, plugin error augment…

bc8fb26

…ations

guybedford added 16 commits August 12, 2018 14:02

remove name handling and cache code

01d73e0

pr feedback

409e2dc

support plugin cache

8e4cd13

wip

768f04b

nocache support, fix up typings and cache persistence

1f37bda

fix up tests

b4269c2

fix up cacheable logic, eviction

5ea0331

typing fix

f35c807

ensure transform cache opt-out for this.cache usage, graph.watchFiles…

7fdaf2c

… refactoring

pad out plugin cache tests

191654f

reverse empty if statement

4f3df7a

attempt memory leak fix

a8cbf42

remote previous memory leak hack

0a90d29

support cacheExpiry config options

11ddef7

support addWatchFile

7ab03b0

pr feedback

09f26fd

guybedford force-pushed the plugin-cache branch from 8ecb1e0 to 164b280 Compare August 12, 2018 12:11

rebase fixes

f649f17

guybedford force-pushed the plugin-cache branch from 164b280 to f649f17 Compare August 12, 2018 12:23

guybedford merged commit 4d491a4 into master Aug 12, 2018

guybedford deleted the plugin-cache branch August 12, 2018 12:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Plugin cache #2389

Plugin cache #2389

guybedford commented Aug 8, 2018 •

edited

Loading

keithamus Aug 8, 2018

guybedford Aug 8, 2018

keithamus Aug 8, 2018

guybedford Aug 8, 2018

shellscape commented Aug 8, 2018

guybedford commented Aug 10, 2018

keithamus Aug 10, 2018

guybedford Aug 10, 2018

guybedford Aug 10, 2018

guybedford commented Aug 10, 2018

keithamus Aug 10, 2018

shellscape Aug 10, 2018

keithamus Aug 10, 2018

guybedford Aug 10, 2018

lukastaegert left a comment

lukastaegert Aug 11, 2018

guybedford Aug 11, 2018

guybedford Aug 11, 2018

lukastaegert Aug 11, 2018

lukastaegert Aug 11, 2018

guybedford Aug 11, 2018

lukastaegert Aug 11, 2018

guybedford Aug 11, 2018

guybedford commented Aug 11, 2018

guybedford commented Aug 11, 2018

lukastaegert commented Aug 11, 2018

tivac commented Aug 12, 2018

lukastaegert commented Aug 12, 2018

guybedford commented Aug 12, 2018

guybedford commented Aug 12, 2018

Plugin cache #2389

Plugin cache #2389

Conversation

guybedford commented Aug 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shellscape commented Aug 8, 2018

guybedford commented Aug 10, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guybedford commented Aug 10, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lukastaegert left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guybedford commented Aug 11, 2018

guybedford commented Aug 11, 2018

lukastaegert commented Aug 11, 2018

tivac commented Aug 12, 2018

lukastaegert commented Aug 12, 2018

guybedford commented Aug 12, 2018

guybedford commented Aug 12, 2018

guybedford commented Aug 8, 2018 •

edited

Loading