Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

perf(nuxt): write templates in single sync step + improve logs #22384

Merged
merged 5 commits into from
Jul 30, 2023
Merged

perf(nuxt): write templates in single sync step + improve logs #22384

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
d723c31
perf(nuxt): write all template files in a synchronous step
danielroe Jul 28, 2023
1cadeb4
fix(nuxt): log and continue template compilation if error
danielroe Jul 28, 2023
f041622
feat: track performance time for template compilation
danielroe Jul 29, 2023
5a90fe5
Merge remote-tracking branch 'origin/main' into perf/templates
danielroe Jul 30, 2023
7f8842d
chore: add comment
danielroe Jul 30, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions packages/kit/src/internal/template.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import { genDynamicImport, genImport, genSafeVariableName } from 'knitwork'
import type { NuxtTemplate } from '@nuxt/schema'

/** @deprecated */
// TODO: Remove support for compiling ejs templates in v4
export async function compileTemplate (template: NuxtTemplate, ctx: any) {
const data = { ...ctx, options: template.options }
if (template.src) {
Expand Down
29 changes: 23 additions & 6 deletions packages/nuxt/src/core/app.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { promises as fsp } from 'node:fs'
import { promises as fsp, mkdirSync, writeFileSync } from 'node:fs'
import { dirname, join, resolve } from 'pathe'
import { defu } from 'defu'
import { compileTemplate, findPath, normalizePlugin, normalizeTemplate, resolveAlias, resolveFiles, resolvePath, templateUtils, tryResolveModule } from '@nuxt/kit'
Expand Down Expand Up @@ -32,13 +32,19 @@ export async function generateApp (nuxt: Nuxt, app: NuxtApp, options: { filter?:
app.templates = app.templates.map(tmpl => normalizeTemplate(tmpl))

// Compile templates into vfs
// TODO: remove utils in v4
const templateContext = { utils: templateUtils, nuxt, app }
await Promise.all((app.templates as Array<ReturnType<typeof normalizeTemplate>>)
const writes: Array<() => void> = []
await Promise.allSettled((app.templates as Array<ReturnType<typeof normalizeTemplate>>)
.filter(template => !options.filter || options.filter(template))
.map(async (template) => {
const contents = await compileTemplate(template, templateContext)

const fullPath = template.dst || resolve(nuxt.options.buildDir, template.filename!)
const mark = performance.mark(fullPath)
const contents = await compileTemplate(template, templateContext).catch((e) => {
console.error(`[nuxt] Could not compile template \`${template.filename}\`.`)
throw e
})

nuxt.vfs[fullPath] = contents

const aliasPath = '#build/' + template.filename!.replace(/\.\w+$/, '')
Expand All @@ -49,12 +55,23 @@ export async function generateApp (nuxt: Nuxt, app: NuxtApp, options: { filter?:
nuxt.vfs[fullPath.replace(/\//g, '\\')] = contents
}

const perf = performance.measure(fullPath, mark?.name) // TODO: remove when Node 14 reaches EOL
const setupTime = perf ? Math.round((perf.duration * 100)) / 100 : 0 // TODO: remove when Node 14 reaches EOL

if (nuxt.options.debug || setupTime > 500) {
console.info(`[nuxt] compiled \`${template.filename}\` in ${setupTime}ms`)
}

if (template.write) {
await fsp.mkdir(dirname(fullPath), { recursive: true })
await fsp.writeFile(fullPath, contents, 'utf8')
writes.push(() => {
mkdirSync(dirname(fullPath), { recursive: true })
writeFileSync(fullPath, contents, 'utf8')
})
}
}))

for (const write of writes) { write() }

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AFAIK, using sync methods, blocks (our) main js render loop but it won't speed up v8 i/o threads. Have you tried with this new batching technique ++ async Promise.all to see timing differences?

Copy link
Member Author

@danielroe danielroe Jul 29, 2023 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've done some tests but the performance benefit I'm looking for here is reducing cascading hmr rather than reducing the time required to write the files.

Copy link
Member

@pi0 pi0 Jul 29, 2023 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I understand that main point 👍🏼 But i suppose (native) HMR watchers are not in the same event loop of main javascript process we are writing files from (unless our receiver is sync again including vite). If either method is faster, it reduces final chance to have duplicate HMR events but if you already tried both, feel free to ignore this comment :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it may be an unnecessary optimisation. I tried both without appreciable time difference, and flagged up the fact that we are actually not watching the buildDir with vite. I will have a look to see if we actually should be watching the directory. Not doing so might be causing some occasionally issues with vite-node cache, for example.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you have a good point here and we should definitely iterate on this. I'd be very open to rethinking this if you notice any issue..

await nuxt.callHook('app:templatesGenerated', app)
}

Expand Down