Commit 646fa4e3eefixed#10547 by
restricting optimisticLookupPackageJson to package.json files with a
"name" property, which effectively skipped over intermediate package.json
files with additional properties.
However, in Node.js 12.16.0 (Meteor 1.9.1+), modules evaluated natively by
Node are considered ECMAScript modules if the closest package.json file
has "type": "module" (or has an .mjs file extension). This poses a problem
for the module.useNode() trick (see packages/modules-runtime/server.js),
because ESM modules cannot be imported using require.
For example, recent versions of the @babel/runtime package have a
@babel/runtime/helpers/esm/package.json file for the ESM versions of its
helpers (which specifies "type": "module"), but that package.json file
does not have a "name" property, because it is not the root package.json
file representing the entire @babel/runtime package.
I considered making the "name" restriction configurable, but that would
have fragmented the caching of optimisticLookupPackageJson. Instead, I
made it return an array of all potentially relevant package.json objects,
which can be safely cached.
This means that the caller has to iterate over the array, but there is
only one call site for this function (in tools/isobuild/package-source.js)
right now, so that wasn't too much work.
https://github.com/meteor/meteor/pull/10772#issuecomment-553517459
The assertion in tools/fs/optimistic.ts was failing if I passed a relative
path for --test-app-path, and passing the path as a second argument when
calling assert made it easier to tell what was going on, so I decided to
keep that change.
In PR #10720, we introduced the makeCheapPathFunction in an effort to
reduce the caching overhead for very frequently called (and already pretty
quick) operations like files.stat.
However, the default maximum LRU cache size of Math.pow(2, 16) can cause
quite a bit of cache eviction churn for large applications, which @veered
has identified as a potential source of build performance problems.
By setting the maximum cache size to Math.pow(2, 20) instead, I am no
longer seeing any files.stat calls in the profiling output for rebuilding
a large internal app, saving several seconds of rebuild time. The obvious
downside is that this cache might accumulate more memory over time, which
is why I didn't just set the max to Infinity, though that might be a
viable option if the total set of paths ever stat'd is small enough to fit
into the available memory.
In the future, I hope to find ways of managing LRU cache size that respond
to actual memory pressure (relative to available memory), rather than
pruning the cache after an arbitrary numeric threshold is reached.
Partial alternative to using json5, which we tried in commit
5124cb495c, which was recently reverted due
to performance concerns: 0e19c365a8.
Should solve the specific problem reported in #10688.
This reverts commit 5124cb495c.
Perhaps unsurprisingly, JSON5 is much slower than JSON, so much so that
"other optimisticReadJsonOrNull" has become the single most expensive leaf
when building one of our internal applications. We will have to find
another solution.
This function determines whether the optimistic caching system should
subscribe to file change notifications for a given path.
Note that Meteor has other ways of watching for file changes (e.g. the
WatchSet abstraction), as well as other mechanisms for invalidating cached
results, so time and resources can often be saved by returning false here,
if we know that the path will be watched in other ways.
Improvements:
- Add METEOR_PROFILE instrumentation to the shouldWatch implementation.
- Stop exporting shouldWatch, since it is no longer used anywhere else in
the codebase.
- Return false early to avoid watching files inside the .meteor directory,
a significant shortcut.
- Allow watching files in application node_modules directories that are
not located directly in the root application directory (closes#10664).
Because json5 is more tolerant of non-standards-compliant input, it has no
trouble with \t tab characters outside of string literals, so this change
should fix issue #10688.
Note that json5 is already installed in dev_bundle/lib/node_modules/json5,
because it is a dependency of @babel/core, so we don't need to rebuild the
dev bundle immediately, though it would be a good idea to do so before the
next beta/RC release.
Unfortunately, this conversion triggered an error due to one of the
shortcomings of the Babel implementation of TypeScript:
SyntaxError: /tools/tool-env/profile.ts: Namespace not marked type-only declare. Non-declarative namespaces are only supported experimentally in Babel. To enable and review caveats see: https://babeljs.io/docs/en/babel-plugin-transform-typescript
278 | }
279 |
> 280 | export namespace Profile {
| ^
281 | export let enabled = !! process.env.METEOR_PROFILE;
282 |
283 | export function time<TResult>(bucket: string, f: () => TResult) {
at File.buildCodeFrameError (/Users/ben/meteor/dev_bundle/lib/node_modules/@babel/core/lib/transformation/file/file.js:261:12)
at transpileNamespace (/Users/ben/meteor/dev_bundle/lib/node_modules/@babel/plugin-transform-typescript/lib/namespace.js:25:25)
at PluginPass.TSModuleDeclaration (/Users/ben/meteor/dev_bundle/lib/node_modules/@babel/plugin-transform-typescript/lib/index.js:271:32)
at newFn (/Users/ben/meteor/dev_bundle/lib/node_modules/@babel/traverse/lib/visitors.js:193:21)
at NodePath._call (/Users/ben/meteor/dev_bundle/lib/node_modules/@babel/traverse/lib/path/context.js:53:20)
at NodePath.call (/Users/ben/meteor/dev_bundle/lib/node_modules/@babel/traverse/lib/path/context.js:40:17)
at NodePath.visit (/Users/ben/meteor/dev_bundle/lib/node_modules/@babel/traverse/lib/path/context.js:88:12)
at TraversalContext.visitQueue (/Users/ben/meteor/dev_bundle/lib/node_modules/@babel/traverse/lib/context.js:118:16)
at TraversalContext.visitMultiple (/Users/ben/meteor/dev_bundle/lib/node_modules/@babel/traverse/lib/context.js:85:17)
at TraversalContext.visit (/Users/ben/meteor/dev_bundle/lib/node_modules/@babel/traverse/lib/context.js:144:19)
