Optimizer: cache runtime artifacts on filesystem #747

sebastianbenz · 2020-05-05T19:31:02Z

This PR addresses the problem that currently transformations rely on working access to the internet:

a new FileSystemCache stores downloaded runtime artifacts (validator rules, latest amp runtime version, v0.css)
transformation never blocks on network access if a cached version is available
download AMP versions have a max-age of 10 minutes, after that we check if a new AMP version is available (not sure if this is a good frequency). If one is available it's downloaded in the background.
option to disable caching (used in tests)
add a postinstall hook that will download the latest runtime artifacts to prime the cache

Fixes #378 #719 #650

@mdmower would be great if you could check if this works on windows.

This will download all runtime parameters once and stores them in an in memory cache.

fstanis

Some additional thoughts:

FileSystemCache has no eviction mechanism and no size limit on values you put in the map. You may run out of memory. I'd limit the number of keys at least (but better to use a proper LRU cache).
You'll make copies of duplicate files (two urls returning same data). This can be avoided you keep an index of url -> hash(data) and use hash(data) instead of hash(url) for the filename.
If you can't write a generic fetch-compatible wrapper, perhaps you can narrow down what fetch does to something simpler, e.g. async function that only takes a URL and returns a string? That would make it easier to wrap in a cached call.

fstanis · 2020-05-05T20:20:11Z

packages/core/lib/FileSystemCache.js

+ */
+'use strict';
+const {join} = require('path');
+const {rmdir, readdir, readFile, writeFile, unlink, existsSync, mkdirSync} = require('fs');


Considering you have this many imports, I'd argue destructuring isn't a good idea, some of these are more readable as part of fs (e.g. fs.unlink vs unlink).

lol - yeah. Frog in boiling water...

fstanis · 2020-05-05T20:21:22Z

packages/core/lib/FileSystemCache.js

+'use strict';
+const {join} = require('path');
+const {rmdir, readdir, readFile, writeFile, unlink, existsSync, mkdirSync} = require('fs');
+const {promisify} = require('util');


Do we still need this? Unless you want to support Node.js 9 and below (which is probably no longer needed... oldest supported LTS is 10), use fs.promises.

Good point. Old habit...fixed.

fstanis · 2020-05-05T20:22:51Z

packages/core/lib/FileSystemCache.js

+const unlinkAsync = promisify(unlink);
+
+class FileSystemCache {
+  static get(


get to me implies a singleton-like pattern, but this actually creates a new instance (also, you used create below for a similar pattern). Also, do you really need this, since it doesn't do much other than provide a default (which you can just do in the constructor).

Renamed to create. Leftover from a previous implementation.

fstanis · 2020-05-05T20:26:59Z

packages/core/lib/FileSystemCache.js

+ * limitations under the License.
+ */
+'use strict';
+const {join} = require('path');


Nit: you're importing path twice. Also, I suggest not destructuring, join can be very confusing (as opposed to path.join).

fstanis · 2020-05-05T20:30:03Z

packages/core/lib/FileSystemCache.js

+    await Promise.all(
+      entries.map((entry) => {
+        let fullPath = path.join(dir, entry.name);
+        return entry.isDirectory() ? this.deleteDir_(fullPath) : unlinkAsync(fullPath);


This is potentially dangerous: directories can be cyclic. That said, I guess this is not a big deal and hard to fix. I'd consider using fs.rmdir which is experimental or rimraf. Or add a max depth.

Actually, if I understand correctly, nested directories are never created by this cache. I'd throw if there are non-files in a directory.

rmdir is unfortunately only Node >=12. rimraf has to many dependencies for a non-dev dependency.

Thinking about this further, there's another danger here: if there's a symlink outside this directory (worst case, to ~ or /), this will delete everything it can. You can argue that's not very likely, but I'd prefer if this was impossible.

A few options:

Ignore directories and only delete files (since directories are never created this way anyway).

Check if a directory is a symlink first.

Resolve symlinks and check if the directory has the initial directory as a prefix.

Regarding fs.rmdir, you can add an if to check if it's available and fallback to your own implementation if not. But probably not needed anyway. Just make sure it doesn't recursively delete my home folder. :)

sg - changed to only delete files in cache dir

packages/optimizer/lib/fetchRuntimeParameters.js

fstanis · 2020-05-05T20:50:21Z

packages/optimizer/lib/fetchRuntimeParameters.js

+/**
+ * @private
+ */
+async function downloadAmpRuntimeStyles_(config, runtimeCssUrl) {


As discussed, consider moving to a more generic fetch wrapper.

packages/optimizer/lib/transformers/AutoExtensionImporter.js

fstanis · 2020-05-05T20:51:09Z

packages/optimizer/scripts/init.js

+
+async function warmupCaches() {
+  // run a dummy transformation to pre-fill the caches
+  await ampOptimizer.transformHtml('<h1>hello world</h1>', {


Nit: just return it.

fstanis · 2020-05-05T21:03:48Z

packages/core/lib/FileSystemCache.js

+    const cacheFile = this.createCacheFileName(key);
+    try {
+      const content = await readFileAsync(cacheFile, 'utf-8');
+      value = JSON.parse(content);


Why must it be JSON? If you use a raw buffer, your cache becomes binary-compatible (and less error prone and less code).

Have you got an example for this?

const content = await fs.readFile(cacheFile); // returns Buffer this.cache.set(key, value); // Buffer in Map ... fs.writeFile(cacheFile, value); // writes Buffer to file, no need to specify encoding

JSON.parse also works on a Buffer, by coercing it to string first (i.e. JSON.parse(str) === JSON.parse(Buffer.from(str))).

Got it, so the advantage is that you don't need to mess with encoding?

ithinkihaveacat · 2020-05-05T21:32:00Z

packages/optimizer/lib/fetchRuntimeParameters.js

+  await initValidatorRules(runtimeParameters, customRuntimeParameters, config);
+  await initRuntimeVersion(runtimeParameters, customRuntimeParameters, config);
+  await initRuntimeStyles(runtimeParameters, config);


These will execute sequentially, but it can be done in parallel with one (annoying) tranformation.

good tip! done

sebastianbenz

Thanks Filip! Comments:

re LRU Cache: not if that's a realistic scenario with releases once a week, but better save than sorry. Using an LRU Cache now.
same value for different URLs is not a use case as we only store version specific artifacts which are immutable.
fetch wrapper: I don't see the need here. There are three different fetches happening (ValidatorRules with built-int fetch, JSON and text), some require a max-age some don't. Unifying this feels like over-engineering at this point. This might change if we need to fetch more artifacts.

sebastianbenz · 2020-05-06T11:50:42Z

packages/optimizer/lib/fetchRuntimeParameters.js

+  await initValidatorRules(runtimeParameters, customRuntimeParameters, config);
+  await initRuntimeVersion(runtimeParameters, customRuntimeParameters, config);
+  await initRuntimeStyles(runtimeParameters, config);


good tip! done

sebastianbenz · 2020-05-06T11:51:16Z

packages/core/lib/FileSystemCache.js

+ */
+'use strict';
+const {join} = require('path');
+const {rmdir, readdir, readFile, writeFile, unlink, existsSync, mkdirSync} = require('fs');


lol - yeah. Frog in boiling water...

sebastianbenz · 2020-05-06T11:54:47Z

packages/core/lib/FileSystemCache.js

+'use strict';
+const {join} = require('path');
+const {rmdir, readdir, readFile, writeFile, unlink, existsSync, mkdirSync} = require('fs');
+const {promisify} = require('util');


Good point. Old habit...fixed.

sebastianbenz · 2020-05-06T12:03:55Z

packages/core/lib/FileSystemCache.js

+const unlinkAsync = promisify(unlink);
+
+class FileSystemCache {
+  static get(


Renamed to create. Leftover from a previous implementation.

sebastianbenz · 2020-05-06T12:07:28Z

packages/core/lib/FileSystemCache.js

+    const cacheFile = this.createCacheFileName(key);
+    try {
+      const content = await readFileAsync(cacheFile, 'utf-8');
+      value = JSON.parse(content);


Have you got an example for this?

packages/optimizer/lib/fetchRuntimeParameters.js

packages/optimizer/lib/transformers/AutoExtensionImporter.js

sebastianbenz · 2020-05-06T12:35:47Z

packages/optimizer/scripts/init.js

+
+async function warmupCaches() {
+  // run a dummy transformation to pre-fill the caches
+  await ampOptimizer.transformHtml('<h1>hello world</h1>', {


sebastianbenz · 2020-05-06T12:35:57Z

packages/core/lib/FileSystemCache.js

+ * limitations under the License.
+ */
+'use strict';
+const {join} = require('path');


sebastianbenz · 2020-05-06T12:37:18Z

packages/core/lib/MaxAge.js

+   * @param {Number} timestampInMs time when max-age value was received
+   * @param {Number} value max-age value in seconds
+   **/
+  static fromJson(timestampInMs, value) {


You're right, renamed to from/toObject. I miss Java in these cases...

fstanis · 2020-05-06T15:02:50Z

packages/core/lib/FileSystemCache.js

+    const cacheFile = this.createCacheFileName(key);
+    try {
+      const content = await readFileAsync(cacheFile, 'utf-8');
+      value = JSON.parse(content);


const content = await fs.readFile(cacheFile); // returns Buffer this.cache.set(key, value); // Buffer in Map ... fs.writeFile(cacheFile, value); // writes Buffer to file, no need to specify encoding

JSON.parse also works on a Buffer, by coercing it to string first (i.e. JSON.parse(str) === JSON.parse(Buffer.from(str))).

fstanis · 2020-05-06T15:10:22Z

packages/core/lib/FileSystemCache.js

+  async set(key, value) {
+    const cacheFile = this.createCacheFileName(key);
+    try {
+      this.cache[key] = value;


I think the test worked because it was always getting a cache miss and loading from a file. You could maybe add a test that clears the folder on the filesystem after a set to make sure it loads it from the LRU.

fstanis · 2020-05-06T15:18:11Z

packages/core/lib/FileSystemCache.js

+    await Promise.all(
+      entries.map((entry) => {
+        let fullPath = path.join(dir, entry.name);
+        return entry.isDirectory() ? this.deleteDir_(fullPath) : unlinkAsync(fullPath);


Thinking about this further, there's another danger here: if there's a symlink outside this directory (worst case, to ~ or /), this will delete everything it can. You can argue that's not very likely, but I'd prefer if this was impossible.

A few options:

Ignore directories and only delete files (since directories are never created this way anyway).

Check if a directory is a symlink first.

Resolve symlinks and check if the directory has the initial directory as a prefix.

Regarding fs.rmdir, you can add an if to check if it's available and fallback to your own implementation if not. But probably not needed anyway. Just make sure it doesn't recursively delete my home folder. :)

sebastianbenz · 2020-05-06T18:02:08Z

@fstanis agree, only removing the files now

Thanks everyone for the review!

sebastianbenz added 4 commits May 5, 2020 13:51

Cache downloaded parameters

45fff17

This will download all runtime parameters once and stores them in an in memory cache.

Cache downloaded runtime artifacts

cb078b3

Warm up cache on postinstall

f3e71f9

add option to disable cache

0dd33e7

sebastianbenz requested review from fstanis and mdmower May 5, 2020 19:31

googlebot added the cla: yes label May 5, 2020

fstanis requested changes May 5, 2020

View reviewed changes

ithinkihaveacat reviewed May 5, 2020

View reviewed changes

addressed comments

9c7835d

sebastianbenz commented May 6, 2020

View reviewed changes

sebastianbenz requested a review from fstanis May 6, 2020 12:56

fstanis approved these changes May 6, 2020

View reviewed changes

only delete files in cache dir

8427903

sebastianbenz force-pushed the memory-cache branch from 2677bb4 to 8427903 Compare May 6, 2020 16:30

sebastianbenz merged commit cd20087 into master May 6, 2020

sebastianbenz added the enhancement label May 6, 2020

herablog mentioned this pull request Jul 21, 2020

Runtime version getting timeout #650

Open

sebastianbenz deleted the memory-cache branch July 21, 2020 08:12

snyk-bot mentioned this pull request Sep 1, 2020

[Snyk] Upgrade debug from 4.1.1 to 4.2.0 southernTrbl/amp-toolbox#4

Open

southernTrbl mentioned this pull request Dec 6, 2023

[Snyk] Security upgrade debug from 4.1.1 to 4.3.1 southernTrbl/amp-toolbox#64

Open

Optimizer: cache runtime artifacts on filesystem #747

Optimizer: cache runtime artifacts on filesystem #747

Conversation

sebastianbenz commented May 5, 2020

fstanis 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

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

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

sebastianbenz 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sebastianbenz commented May 6, 2020