[Bugfix] Ensure stability of filename cache-keys #909

stefanpenner · 2021-06-09T23:45:38Z

[Bugfix] Ensure stability of filename cache-keys

JSON.stringify(structure) isn’t inherently stable as it relies on various
internal details of how structure was created.

As written, if a given babel configuration is create in an dynamic manner,
it is possible for babel-loader to have spurious cache misses.

To address this, we can use one of the many stable stringify alternatives.

For this PR I have selected fast-json-stable-stringify
for that task, as it appears both popular and it’s benchmarks look promising.

This PR does not explicitly include tests, as testing this is both tricky
to test in this context, and the important tests are contained within
fast-json-stable-stringify itself.

Please Read the CONTRIBUTING Guidelines
In particular the portion on Commit Message Formatting

Please check if the PR fulfills these requirements

Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)

What is the current behavior? (You can also link to an open issue here)

cache keys are unstable, causing occasional cache misses

What is the new behavior?

cache keys should be stable

Does this PR introduce a breaking change?

Yes
No

Impact: improved cache hit frequency
Migration path for existing applications: none
Github Issue(s) this is regarding: none

nicolo-ribaudo · 2021-06-09T23:50:03Z

Thanks! Is it possible that the CI failure is related to this change?

stefanpenner · 2021-06-10T00:00:26Z

@nicolo-ribaudo failure is occurring locally too, I will investigate & address (most likely later tonight, or tomorrow).

stefanpenner · 2021-06-10T16:54:58Z

@nicolo-ribaudo quick-update I'll do some additional debugging this afternoon, but my initial debugging pass suggests this test failure may actually be an example of an erroneous cache miss now corrected with this change. I'm not yet 100% convinced, but intend to continue investigation this afternoon.

stefanpenner · 2021-06-10T19:38:09Z

test/cache.test.js

@@ -376,7 +376,7 @@ test.cb("should allow to specify the .babelrc file", t => {

    fs.readdir(t.context.cacheDirectory, (err, files) => {
      t.is(err, null);
-      t.true(files.length === 2);
+      t.true(files.length === 1);


This changes because the cached files now have an identical key.

Well, it's good that we can actually see the improvement caused by this change 😄

stefanpenner · 2021-06-10T19:41:35Z

@nicolo-ribaudo I believe in-fact the test failures where an example of an unstable cache key.

nicolo-ribaudo

Thanks!

stefanpenner · 2021-06-11T16:42:52Z

@hzoo / @nicolo-ribaudo what are the next steps? Anything I can help with?

JLHwung · 2021-06-11T17:19:54Z

fast-json-stable-stringify is a new dependency. I am auditing the source of fast-json-stable-stringify. The serialization is applied on every compile file so performance is crucial here.

I clone the repo and here the benchmark result. It seems that fast-stable-stringify is fastest.

fast-json-stable-stringify x 21,781 ops/sec ±1.09% (87 runs sampled)
json-stable-stringify x 16,813 ops/sec ±0.92% (92 runs sampled)
fast-stable-stringify x 25,183 ops/sec ±0.74% (93 runs sampled)
faster-stable-stringify x 20,960 ops/sec ±1.21% (93 runs sampled)

stefanpenner · 2021-06-11T17:28:03Z

@JLHwung I can switch to that 1, give me a sec.

`JSON.stringify(structure)` isn’t inherently stable as it relies on various internal details of how `structure` was created. As written, if a given babel configuration is create in an dynamic manner, it is possible for babel-loader to have spurious cache misses. To address this, we can use one of the many stable stringify alternatives. For this PR I have selected [fast-stable-stringify](https://www.npmjs.com/package/fast-stable-stringify) for that task, as it appears both popular and it’s benchmarks look promising. This PR does not explicitly include tests, as testing this is both tricky to test in this context, and the important tests are contained within fast-stable-stringify itself.

JLHwung · 2021-06-11T17:32:52Z

fast-stable-stringify seems not actively maintained: https://github.com/nickyout/fast-stable-stringify I lean to host our own fast-stable-stringify for babel libraries only.

nicolo-ribaudo · 2021-06-11T17:37:54Z

There haven't been new commits, but there are no reported bugs. It could be "maintianed, but working".

We should only self-host it if we find a bug that we need to fix, and the maintainer doesn't accept PRs.

stefanpenner · 2021-06-11T17:38:40Z

I agree with @nicolo-ribaudo's POV.

JLHwung · 2021-06-11T18:37:20Z

Designing general stable JSON.stringify is more difficult than designing specific stable serializer for Babel options. Yes I use the term serializer because we don't have to generate a valid JSON. After all we don't read the serialized results and parse them back. We want to make sure for most different options, the hash is different. So we can perform better than most stable JSON stringify. For example,

We can stringify any object key v with "\"" + v + "\"".
If value is a string, we use it without escaping " and \, they are required by JSON but we don't have to output json.

Combining these two tricks I see 150% improvement (named as fast-stable-stringify-2) on the benchmark and the new serializer is now 20% slower than the native JSON.stringify:

fast-json-stable-stringify x 22,088 ops/sec ±1.16% (87 runs sampled)
fast-stable-stringify x 23,703 ops/sec ±1.59% (85 runs sampled)
fast-stable-stringify-2 x 61,288 ops/sec ±1.91% (83 runs sampled)
Native JSON.stringify x 69,951 ops/sec ±1.15% (93 runs sampled)
The fastest is Native JSON.stringify

The serializer could be faster than the native JSON.stringify because we don't scan strings. We just make sure every object can map to a string.

I have pushed the branch in https://github.com/JLHwung/fast-stable-stringify/tree/babel

stefanpenner · 2021-06-11T20:38:37Z

@JLHwung additional performance here sounds great.

stefanpenner · 2021-06-14T15:52:46Z

@JLHwung et.al what are the next steps?

JLHwung · 2021-06-14T18:39:05Z

My personal opinion here: Since we don't have to use JSON stringify, I think we can host our own property stable config serializer. Now that the cache key is updated and the old cache should probably be invalidated, I think we should inform users on the next release note that we recommend running rm -rf ./node_modules/.cache/babel-loader before upgrading babel-loader.

stefanpenner · 2021-06-14T21:28:54Z

@JLHwung let me know when your variant of stringification is published, and I can include it here...

JLHwung · 2021-06-14T21:30:31Z

@stefanpenner We don't have to publish a new package, since it is not JSON and meant to be used in babel-loader only, we can inline it in this repo.

hzoo · 2021-06-15T00:37:15Z

I'm down with in-lining our changes as in JLHwung/fast-stable-stringify@f5b0ca9 in the repo

stefanpenner · 2021-06-15T23:11:50Z

ok

stefanpenner force-pushed the fix-cache-keys branch from 6fb3ede to 593dca6 Compare June 10, 2021 19:37

stefanpenner commented Jun 10, 2021

View reviewed changes

nicolo-ribaudo approved these changes Jun 10, 2021

View reviewed changes

nicolo-ribaudo requested a review from JLHwung June 10, 2021 21:24

hzoo approved these changes Jun 11, 2021

View reviewed changes

stefanpenner force-pushed the fix-cache-keys branch from 593dca6 to fde5dfd Compare June 11, 2021 17:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Bugfix] Ensure stability of filename cache-keys #909

[Bugfix] Ensure stability of filename cache-keys #909

stefanpenner commented Jun 9, 2021 •

edited

nicolo-ribaudo commented Jun 9, 2021 •

edited

stefanpenner commented Jun 10, 2021 •

edited

stefanpenner commented Jun 10, 2021

stefanpenner Jun 10, 2021

nicolo-ribaudo Jun 10, 2021

stefanpenner commented Jun 10, 2021 •

edited

nicolo-ribaudo left a comment

stefanpenner commented Jun 11, 2021

JLHwung commented Jun 11, 2021 •

edited

stefanpenner commented Jun 11, 2021

JLHwung commented Jun 11, 2021

nicolo-ribaudo commented Jun 11, 2021

stefanpenner commented Jun 11, 2021

JLHwung commented Jun 11, 2021 •

edited

stefanpenner commented Jun 11, 2021

stefanpenner commented Jun 14, 2021

JLHwung commented Jun 14, 2021

stefanpenner commented Jun 14, 2021

JLHwung commented Jun 14, 2021

hzoo commented Jun 15, 2021 •

edited

stefanpenner commented Jun 15, 2021

[Bugfix] Ensure stability of filename cache-keys #909

Are you sure you want to change the base?

[Bugfix] Ensure stability of filename cache-keys #909

Conversation

stefanpenner commented Jun 9, 2021 • edited

nicolo-ribaudo commented Jun 9, 2021 • edited

stefanpenner commented Jun 10, 2021 • edited

stefanpenner commented Jun 10, 2021

stefanpenner Jun 10, 2021

Choose a reason for hiding this comment

nicolo-ribaudo Jun 10, 2021

Choose a reason for hiding this comment

stefanpenner commented Jun 10, 2021 • edited

nicolo-ribaudo left a comment

Choose a reason for hiding this comment

stefanpenner commented Jun 11, 2021

JLHwung commented Jun 11, 2021 • edited

stefanpenner commented Jun 11, 2021

JLHwung commented Jun 11, 2021

nicolo-ribaudo commented Jun 11, 2021

stefanpenner commented Jun 11, 2021

JLHwung commented Jun 11, 2021 • edited

stefanpenner commented Jun 11, 2021

stefanpenner commented Jun 14, 2021

JLHwung commented Jun 14, 2021

stefanpenner commented Jun 14, 2021

JLHwung commented Jun 14, 2021

hzoo commented Jun 15, 2021 • edited

stefanpenner commented Jun 15, 2021

stefanpenner commented Jun 9, 2021 •

edited

nicolo-ribaudo commented Jun 9, 2021 •

edited

stefanpenner commented Jun 10, 2021 •

edited

stefanpenner commented Jun 10, 2021 •

edited

JLHwung commented Jun 11, 2021 •

edited

JLHwung commented Jun 11, 2021 •

edited

hzoo commented Jun 15, 2021 •

edited