perf: deallocate out-of-scope references #32162
Conversation
|
|
@nuxt/kit
nuxt
@nuxt/rspack-builder
@nuxt/schema
@nuxt/vite-builder
@nuxt/webpack-builder
commit: |
CodSpeed Performance ReportMerging #32162 will not alter performanceComparing Summary
|
|
Hey @danielroe ! This PR would optimize memory utilization of the dev server and the prod build process, because it will allow references to be GC'ed before the process exits (as only then the modules (files) would be unloaded). Especially for the dev server, which runs indefinitely until explicitly shut down, this can be significant for constrained environments, as otherwise the data would be held in memory indefinitely. However, in case of a Realistically speaking, a I was wondering what your thoughts are on this trade-off? Would you still like this PR to be pushed? |
WalkthroughThis set of changes primarily refactors multiple modules by relocating utility functions, constants, and regular expressions from the module or file top-level scope into the more local scopes of the functions or methods where they are used. This includes moving helper functions and constants into the bodies of setup functions, template generators, or main logic functions in various Nuxt and Vite packages. No changes are made to the external APIs or exported function signatures; all modifications are internal, affecting only code organisation and scope. The core logic and behaviour of the affected modules remain unchanged. Note ⚡️ AI Code Reviews for VS Code, Cursor, WindsurfCodeRabbit now has a plugin for VS Code, Cursor and Windsurf. This brings AI code reviews directly in the code editor. Each commit is reviewed immediately, finding bugs before the PR is raised. Seamless context handoff to your AI code agent ensures that you can easily incorporate review feedback. ✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/nuxt/src/core/app.ts (1)
64-80: Consider replacingdeletewithundefinedassignment for better performance.The function relocation is good for memory optimization. However, the
deleteoperator can impact performance.Apply this diff to improve performance:
- async function compileTemplate<T> (template: NuxtTemplate<T>, ctx: { nuxt: Nuxt, app: NuxtApp, utils?: unknown }) { - delete ctx.utils + async function compileTemplate<T> (template: NuxtTemplate<T>, ctx: { nuxt: Nuxt, app: NuxtApp, utils?: unknown }) { + ctx.utils = undefined🧰 Tools
🪛 Biome (1.9.4)
[error] 65-65: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
packages/kit/src/template.ts(3 hunks)packages/nuxt/src/components/module.ts(1 hunks)packages/nuxt/src/components/scan.ts(1 hunks)packages/nuxt/src/components/templates.ts(3 hunks)packages/nuxt/src/core/app.ts(3 hunks)packages/nuxt/src/core/external-config-files.ts(1 hunks)packages/nuxt/src/core/nitro.ts(1 hunks)packages/nuxt/src/core/nuxt.ts(6 hunks)packages/nuxt/src/core/templates.ts(4 hunks)packages/nuxt/src/head/module.ts(1 hunks)packages/nuxt/src/imports/module.ts(1 hunks)packages/nuxt/src/pages/module.ts(2 hunks)packages/vite/src/css.ts(1 hunks)packages/vite/src/vite.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (4)
packages/nuxt/src/components/module.ts (1)
packages/nuxt/src/utils.ts (1)
isDirectory(9-11)
packages/nuxt/src/components/scan.ts (1)
packages/nuxt/src/utils.ts (1)
logger(13-13)
packages/nuxt/src/imports/module.ts (6)
packages/schema/src/types/imports.ts (1)
ImportsOptions(3-41)packages/kit/src/index.ts (5)
useNuxt(21-21)directoryToURL(35-35)resolveAlias(28-28)tryResolveModule(35-35)addTypeTemplate(31-31)packages/kit/src/internal/esm.ts (2)
directoryToURL(13-15)tryResolveModule(26-32)packages/kit/src/resolve.ts (1)
resolveAlias(74-77)packages/nuxt/src/utils.ts (1)
isDirectory(9-11)packages/kit/src/template.ts (1)
addTypeTemplate(71-93)
packages/kit/src/template.ts (1)
packages/nuxt/src/core/utils/index.ts (1)
EXTENSION_RE(19-19)
🪛 Biome (1.9.4)
packages/nuxt/src/core/app.ts
[error] 65-65: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
🔇 Additional comments (31)
packages/vite/src/vite.ts (1)
240-247: LGTM: Function relocation for memory optimization.Moving the
withLogshelper function into thebundlefunction scope aligns well with the PR's memory optimization objective. Since this utility is only used withinbundle, localising its scope will allow proper garbage collection when the function execution completes.packages/vite/src/css.ts (1)
17-20: LGTM: Good encapsulation and memory optimization.Relocating
sortPluginsinto theresolveCSSOptionsfunction scope improves encapsulation whilst supporting the memory optimization goals. The function is only used within this context, making the scope reduction appropriate.packages/kit/src/template.ts (4)
150-153: LGTM: Utility function relocation improves locality.Moving
relativeWithDotinto_generateTypesis appropriate since it's only used within this function. The implementation correctly handles relative path formatting with proper dot prefixing.
277-279: LGTM: Constants relocated appropriately.The
EXTENSION_REconstant andexcludedAliasarray are correctly moved into local scope. Note that whilst there's a simplerEXTENSION_REinpackages/nuxt/src/core/utils/index.ts, this more specific version for TypeScript declaration files is appropriate for this context.
360-369: LGTM: Helper function properly encapsulated.The
sortTsPathsfunction relocation is well-placed within_generateTypeswhere it's exclusively used. The function correctly handles the reordering of TypeScript paths to ensure#buildkeys are positioned at the end.
377-387: LGTM: Template rendering utilities appropriately scoped.Moving
renderAttrsandrenderAttrinto the_generateTypesfunction improves encapsulation. These utilities are only needed for generating TypeScript reference comments within this specific context.packages/nuxt/src/components/module.ts (1)
35-43: LGTM: Utility functions appropriately relocated to local scope.Moving these helper functions and constants into the
setupfunction scope is excellent for memory optimization whilst maintaining functionality. All utilities are exclusively used within this context:
isPureObjectOrString,isDirectory, and regex constants are well-encapsulated- The synchronous
isDirectoryimplementation usingstatSyncis appropriate here (note there's an async version inpackages/nuxt/src/utils.tsfor different use cases)compareDirByPathLengthmaintains proper component directory sorting logicpackages/nuxt/src/head/module.ts (1)
16-16: Scope relocation improves memory utilisation.Moving the
componentsarray into thesetupfunction scope allows it to be garbage collected after module initialisation completes, which aligns with the PR's memory optimisation goals.packages/nuxt/src/components/scan.ts (1)
29-41: Excellent scope optimisation for memory management.Moving the regex constants and helper function into the
scanComponentsfunction scope is a well-executed optimisation. These utilities are only used within this function, and localising them allows for proper garbage collection after component scanning completes.packages/nuxt/src/imports/module.ts (1)
168-228: Beneficial function relocation for memory optimisation.Moving
addDeclarationTemplatesinto thesetupfunction scope is an excellent optimisation. Since this function is only called once during module setup, localising it allows all its internal state and closures to be properly garbage collected after the setup phase completes.packages/nuxt/src/core/templates.ts (4)
106-109: Good scope localisation for regex constants.Moving these regex constants into the
pluginsDeclarationtemplate function where they're used allows for better memory management during the build process.
186-187: Appropriate scope relocation for template-specific constants.Localising these regex constants within the
schemaTemplatefunction aligns with the PR's memory optimisation goals and keeps related logic together.
329-339: Excellent helper function localisation.Moving the
renderAttrandrenderAttrshelper functions into thenitroSchemaTemplatewhere they're exclusively used is a smart optimisation that will allow proper garbage collection after template generation.
595-596: Consistent pattern application for regex constants.The relocation of these regex constants into the
buildTypeTemplatefunction maintains consistency with the overall refactoring pattern and improves memory utilisation.packages/nuxt/src/components/templates.ts (3)
81-88: Excellent memory optimisation.Moving
createImportMagicCommentsinto the local scope ofcomponentsIslandsTemplate.getContentsallows for proper garbage collection after template generation. This aligns perfectly with the PR's goal of deallocating out-of-scope references.
23-30: Good refactoring for memory efficiency.Inlining the
emptyComponentsPluginconstant directly within the conditional improves memory usage by avoiding a module-level constant that would persist throughout the application lifecycle.
109-109: Proper localisation of regex constant.Moving
NON_VUE_REinto the local scope ensures it's only allocated when needed and can be garbage collected after the template generation completes.packages/nuxt/src/pages/module.ts (2)
36-55: Excellent scope optimisation.Moving
OPTIONAL_PARAM_RE,runtimeDir, andresolveRouterOptionsinto the setup function scope allows for proper memory management. These utilities are only needed during module initialisation and can be garbage collected afterwards.
544-576: Good localisation of HMR utilities.Moving
ROUTES_HMR_CODEandhandleHotUpdateinto the local scope near their usage point improves memory efficiency whilst maintaining all HMR functionality. The code structure remains clear and maintainable.packages/nuxt/src/core/external-config-files.ts (1)
5-59: Excellent memory optimisation refactoring.Moving the interface definition and all helper functions into the scope of
checkForExternalConfigurationFilessignificantly improves memory efficiency. These utilities are only needed during the configuration check phase and can be properly garbage collected afterwards. The refactoring maintains all existing logic, error handling, and warning messages without any functional changes.packages/nuxt/src/core/nitro.ts (3)
25-30: Excellent regex and utility localisation.Moving
NODE_MODULES_RE,PNPM_NODE_MODULES_RE,RELATIVE_RE, andrelativeWithDotinto theinitNitroscope provides significant memory benefits. These utilities are only needed during Nitro initialisation and can be garbage collected afterwards.
32-36: Good scope optimisation for log level mapping.Localising
logLevelMapReversewithininitNitroensures this constant is only allocated when needed and can be properly garbage collected after initialisation.
38-70: Superb SPA template handling refactoring.Moving
spaLoadingTemplatePathandspaLoadingTemplatefunctions into the local scope provides excellent memory optimisation whilst maintaining all functionality:
- Proper error handling for missing files
- Correct fallback logic for different configuration options
- Template caching and resolution logic preserved
- Warning messages for invalid configurations remain intact
This refactoring exemplifies the PR's memory optimisation goals.
packages/nuxt/src/core/app.ts (2)
44-48: LGTM! Good memory optimization.Moving the
postTemplatesset into thegenerateAppfunction scope allows it to be garbage collected after the function completes, which aligns with the PR's memory optimization objectives.
218-231: LGTM! Appropriate function scoping.Moving the
resolvePathshelper function intoresolveAppwhere it's exclusively used is a good optimization. This allows the function to be garbage collected afterresolveAppcompletes.packages/nuxt/src/core/nuxt.ts (6)
158-160: LGTM! Appropriate constant scoping.Moving the
fallbackCompatibilityDateconstant into theinitNuxtfunction scope where it's used is consistent with the PR's memory optimization strategy.
209-216: LGTM! Good memory optimization for nightlies mapping.The
nightliesobject is appropriately scoped withininitNuxtwhere it's exclusively used for resolving nightly package paths.
422-510: LGTM! Well-structured module resolution helpers.The
resolveModuleandresolveModulesfunctions are appropriately scoped withininitNuxt. This encapsulation improves code organization whilst allowing these helper functions to be garbage collected after initialization.
752-752: LGTM! Appropriate regex scoping.Moving the
RESTART_REregex intoinitNuxtwhere it's used for watch pattern matching is consistent with the memory optimization approach.
828-841: LGTM! Effective array deduplication scoping.The
deduplicateArrayfunction is appropriately scoped withinloadNuxtwhere it's used for deduplicating configuration arrays.
908-934: LGTM! Well-scoped property portal creation.The
createPortalPropertiesfunction is correctly scoped withinloadNuxtwhere it's used to create shared getter/setter properties between Nuxt and Nitro configurations.
|
I think we can optimise for a single start rather than frequent reload. Having said that (and this isn't a reason not to merge this one) I think PRs like these are likely to lead to very little improvement to the actual performance of Nuxt. (while there is certainly low-hanging fruit that would make a much bigger difference) more generally, some of these PRs lead to improvement but it's just a tiny amount in a function that's not rerun many times. it doesn't mean it's not worth doing but the risk of accidentally breaking something is higher than the benefit it can often bring in future, would you be able to create more targeted benchmarks before creating the PR, which can validate the approach you want to take? otherwise there's probably an unlimited amount of bikeshedding we could do, and I'd rather spend our time reviewing/coding on changes that will make a more significant difference. 🙏 please don't take this as a lack of appreciation - I really value your time on this, but want to make best use of it as well as the time of the rest of the team |
|
Hey @danielroe ! I appreciate your response. First of all, I do not take it as lack of appreciation, I fully understand your point. TL;DR if you could assist with making reliable memory utilization benchmarks that would be appreciated, because I am uncertain of how to do that. Additionally, the smaller changes would really have a compounding effect that in the long term could be much more significant than what can be seen in the short term. In depth: The main issue is that such changes are mainly tackling memory efficiency. Unlike runtime performance, which can be measured with I can, however, explain this logically with an extreme pseudo-code. // without deallocation, every top-level function/object is in memory until the module is unloaded,
// which is usually the end of the process
// for the nuxt dev server, which runs indefinitely, this means until the user shuts it off
const functionOnly = { ...2GB worth of data }
function func() {
// something with functionOnly
}
func()
const largeObject = { ...7GB worth of data } // the program crashed, because we
// ran out of memory, 2GB from `functionOnly` and 7GB from `largeObject`// with deallocation, every function/object is cleared from memory by the garbage collector (GC)
// when the scope in which it was defined is no longer reachable
// this means no memory footprint pollution (data isn't left in memory longer than it needs to be)
// for the nuxt dev server this is substantial, because it allows for far greater memory safety
// and gives more room for the actual end-developer's application
function func() {
const functionOnly = { ...2GB worth of data }
// something with functionOnly
}
func()
// `functionOnly` is now GC'ed
const largeObject = { ...7GB worth of data } // the program doesn't crash,
// because `functionOnly` was GC'ed, we now have 1GB of free memory for user codeAs for the tiny amount of improvements in some of the PR's, I also agree that they may not be the most substantial, but there are a lot of such possible changes. What really makes it substantial is the compounding effect of these multiple small changes that in the long term lead to much bigger improvements. Not to mention, there is definitely much more significant low hanging fruits, as you said, that I, and seemingly everyone else, are simply yet to reach, because some of them are just harder to spot. I can say about myself that the optimizations of reducing duplicate function calls that I made a couple months ago were much easier for me to spot and implement than the optimizations I am working on today, such as this one. Both because it's much less obvious and because it delves deeper into the fundamentals of concepts like memory and performance. However, the fact that these low hanging fruits weren't tackled yet doesn't necessarily have to deter us from tackling the smaller changes that we do notice, because otherwise it could become technical debt, if we end up tackling all the significant fruits and being left with only the less significant ones. |
|
I understand that this PR is about memory management. My comment (below the horizontal rule) was more general. I am worried that it's possible for large PRs to have a larger likelihood of introducing bugs and taking up time reviewing them, than the potential benefit. I review PRs line-by-line and this takes a considerable amount of time for PRs that might make only a 0.1ms difference in a user's experience. I'm proposing using benchmarks to make it more obvious which changes matter. We can do the same for memory management, with profiling. |
|
I appreciate your response! I'll try to set up profiling similar to the one I made in #29620. That's the most reliable profiler I managed to find. I'll post the results below. If you'd like to backlog this PR, that's perfectly fine with me, I understand that you may have other priorities, and I'm of course not trying to force any PR to get merged against someone's will. I'm just trying to help and optimize wherever I can because I believe it will, in the long term, benefit everybody 🙂 As for the bugs, I think it's safe to assume that since all the tests passed, it hasn't introduced any breakage |
|
Just a quick note on profiling, I'd like to run it on a real nuxt server. We can, for example, trigger HMR or trigger restarts with Nuxt hooks. If you want, I'd advise checking vite out - they added profiling support and then used it to speed up the server significantly. |
🔗 Linked issue
A reiteration of #29661 on the latest main branch.
This iteration goes further to optimize related internally-used functions in core functions that execute only once, and reverts previously "optimized" parts that do not benefit from the optimization due to global reference dependencies/closures (hooks, watchers, plugins, etc)
📚 Description