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: performance overhaul #25771

Draft
wants to merge 141 commits into
base: main
Choose a base branch
from

Conversation

GalacticHypernova
Copy link
Contributor

@GalacticHypernova GalacticHypernova commented Feb 13, 2024

NOTE:

This is the previous pr, new one is at #26114 .
This one had some unexplained issues with certain commits (for example, #25771 (comment)) and overall had some edge cases, so I have decided to stop commiting on this one and opening the new one to take care of all the issues more carefully.

πŸ”— 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 is a PR that aims to improve both build time and runtime performance to the max by refactoring everything that comes to mind with performance best practices, some of which include smarter and fewer object/array iterations, reduced indexes, smarter variable usage for constant values, and more.

Some key improvements in this PR:

  • Refactors of known costly javascript methods:
    In some places throughout the app, there are operations that rely on certain javascript methods that are known to be more expensive at certain conditions than manual implementation
    (similar to endsWith in perf(vite): rework endsWith to direct indexΒ #24746
    and startsWith in perf(kit, schema, nuxt): rework startsWith to direct indexΒ #24744).
    One key change under this category is the usage of Array.prototype.includes.
    Array.prototype.includes appears to be slower when checking for existence of a certain input between at the very least 2 elements than directly checking each element with strict equality (like when dealing with strings).
    After thorough benchmarking, both browser and node, direct comparisons showed marginally better performance, benchmarks attached below in that same order with the following benchmark:

    const a = "#text"
    console.time("start")
    for(let i = 0; i< 1000000; i++) {
      const inc = ['#comment', '#text'].includes(a)
    }
    console.timeEnd("start")
    console.time("stop")
    for(let i = 0; i< 1000000; i++) {
      const inc  = a === "#comment" || a === "#text"
    }
    console.timeEnd("stop")

    Browser:

    image

    Node:

    image

  • Reduced and reworked iterations
    Originally the main change in this PR, many parts in nuxt involve some sort of array/object manipulations, and in some places they are either repeated, redundant, or simply costly in terms of performance. The main examples of such cases are the small side PR's I have submitted like perf(kit): avoid duplicate join operationΒ #24717, perf(nuxt): avoid duplicate iterations over layersΒ #24730, and numerous others. I couldn't get all of it at the same time so I have decided to group the rest here, and also handle the iteration logic as opposed to mere count.

  • Reduced indexes:
    Some places in nuxt include working with indexes, namely the length property of array/strings. While that alone isn't expensive, it still makes that repeated index, which especially with bigger inputs (like in HTML parsing or AST traversal) will gradually cost more and more microseconds, and might eventually become slightly noticeable in page performance.

  • Early returns
    Similar to perf: don't manipulate an empty valueΒ #25647, there are some places where some iterations are made on potentially empty values, which increase the overhead and therefore decrease performance. This PR adds guards to ensure there won't be iterations over an empty value.

πŸ“ Checklist

  • I have linked an issue or discussion.
  • I have added tests (if possible).
  • I have updated the documentation accordingly.

Copy link

stackblitz bot commented Feb 13, 2024

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

@GalacticHypernova
Copy link
Contributor Author

GalacticHypernova commented Mar 2, 2024

Still need to figure out why the dev builds fail...
It doesn't make much sense that 44d3c19 caused the 8 dev builds to fail for the first time, it just can't be.

@GalacticHypernova
Copy link
Contributor Author

Update: I decided to rework this more carefully, I will open a new PR for it and close this one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant