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.

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

perf(schema): avoid duplicate get operations #24734

Merged
merged 9 commits into from Dec 15, 2023

Conversation

GalacticHypernova
Copy link
Contributor

@GalacticHypernova GalacticHypernova commented Dec 13, 2023

πŸ”— Linked issue

❓ Type of change

  • πŸ“– Documentation (updates to the documentation, readme or JSdoc annotations)
  • 🐞 Bug fix (a non-breaking change that fixes an issue)
  • πŸ‘Œ Enhancement (improving an existing functionality like performance)
  • ✨ New feature (a non-breaking change that adds functionality)
  • 🧹 Chore (updates to the build process or auxiliary tools and libraries)
  • ⚠️ Breaking change (fix or feature that would cause existing functionality to change)

πŸ“š Description

This PR saves the app that is gotten through the runtimeConfig $resolve async function and is used in place of the current 3 calls, resulting in a ~99.1% increase in performance.
Benchmark results (tested with the smallest possible timeout of 0 ms which would, in real time, likely take longer than that):

async function get(name){
        await new Promise((resolve)=>{
            setTimeout(()=>{},0)
            resolve(void 0)
        })
        return {baseURL:"",buildAssetsDir:"",cdnURL:""}
    }
    const val={}
    console.time("current")
    for (let index = 0; index < 100000; index++) {
       val.baseURL=(await get('app')).baseURL
       val.buildAssetsDir=(await get('app')).buildAssetsDir
       val.cdnURL=(await get('app')).cdnURL
    }
    console.timeEnd("current")
    const app = await get('app')
    console.time("PR")
    for (let index = 0; index < 100000; index++) {
        val.baseURL=app.baseURL
        val.buildAssetsDir=app.buildAssetsDir
        val.cdnURL=app.cdnURL
     }
     console.timeEnd("PR")

image

πŸ“ Checklist

  • I have linked an issue or discussion.
  • I have updated the documentation accordingly.

Copy link

stackblitz bot commented Dec 13, 2023

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

@github-actions github-actions bot added the 3.x label Dec 13, 2023
@GalacticHypernova
Copy link
Contributor Author

I will try to get the rest handled as well, but this was the thing that caught my eye the most.

Copy link
Member

@danielroe danielroe left a comment

Choose a reason for hiding this comment

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

Nice πŸ‘Œ

@GalacticHypernova
Copy link
Contributor Author

Could I request this PR to not be merged until I'm done fixing all of schema? Or is it fine if it gets merged in chunks?

Copy link
Member

Oh, of course.

@danielroe danielroe marked this pull request as draft December 14, 2023 11:00
@GalacticHypernova
Copy link
Contributor Author

Thank you!
I'll try to get this update ready by the end of the day.

@GalacticHypernova
Copy link
Contributor Author

GalacticHypernova commented Dec 14, 2023

Looking at the webpack schema, I don't think making it as such is necessarily the most performant (for the same reason as this PR's title):
image

Can this be changed to something like this?

vue:  {
    $resolve: async (val, get) => {
        const vue = await get('vue')
        return defu(val, {
            transformAssetUrls: {
              video: 'src',
              source: 'src',
              object: 'src',
              embed: 'src'
            },
            compilerOptions: vue.compilerOptions,
            propsDestructure: Boolean(vue.propsDestructure),
            defineModel: Boolean(vue.defineModel)
        })
    }
}

Or would it have some kind of side effects? Because the same thing is done with the runtimeConfig:
image
It provides it from the app already so it doesn't have to get everything separately.

@GalacticHypernova GalacticHypernova marked this pull request as ready for review December 15, 2023 08:38
@danielroe danielroe merged commit 24bedc5 into nuxt:main Dec 15, 2023
34 checks passed
@github-actions github-actions bot mentioned this pull request Dec 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants