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(runtime-core): improve efficiency of normalizePropsOptions #11409

Merged
merged 2 commits into from
Jul 29, 2024

Conversation

skirtles-code
Copy link
Contributor

@skirtles-code skirtles-code commented Jul 20, 2024

Closes #9739.

The result of normalizePropsOptions is cached, so the performance usually doesn't matter too much. But there are two cases where the cache may not help:

  • SSR. The cache is per application instance, so a new cache is used for each request.
  • Components created on-the-fly, e.g. by defining them inside the <script setup> block of another component.

A lot of time in normalizePropsOptions is spent in getType, trying to figure out the values of the shouldCast and shouldCastTrue flags. I go into a bit more detail about that at #9739 (comment).

The Playground isn't necessarily the best tool for benchmarking, but here's an example to try to illustrate that my changes have improved the performance:

I based my branch on 3.4.33, so it should be a fair comparison. It seems to be faster in both Chrome and Firefox (I didn't try other browsers).

The Playground example isn't very 'real'. It's designed to focus on the performance of normalizePropsOptions as much as possible, so the performance benefit would be much less in real applications. But if performance can be improved without hurting bundle size or maintainability, it seems worth it to me.

The Playground uses small arrays for the type. I think that's representative of real code. I didn't think it would be realistic to test with arrays of hundreds or thousands of items, so my conclusions are all based on the assumption that the arrays will be relatively short.

Most of my benchmarking I did using Node instead, calling normalizePropsOptions directly. Using a similar props definition to the one in the Playground above I saw an improvement of about 35%. There's a version of the code I used at:

That's using arrays for the prop types. I also tested using other values, such as type: String. The performance still seemed slightly better, maybe about 10% better, but it was less clear relative to the background noise in the measurements.

I also tried to reduce the size of the build. There was a lot of code involved in performing the shouldCast/shouldCastTrue checks, much of which is no longer needed. I experimented with various implementations, testing both the performance and build size. The first commit on this PR, 3f37cf7, gave a slightly smaller build size (saving 176 bytes rather than 116), but it wasn't quite as fast as the second version (about 30% faster than 3.4.33, rather than 35%).


The original motivation for these changes came from #9739, which proposed adding a cache to getType. Since that PR was opened there have been other changes to getType via #10327. I think those other changes make adding a cache less effective. The remaining problem is just that getType gets called so much, which is essentially the problem I'm attempting to address here.


One of the changes introduced by #10327 was this code:

} else if (typeof ctor === 'object') {
// Attempting to directly access constructor name if possible
const name = ctor.constructor && ctor.constructor.name
return name || ''
}

I'm not entirely sure why this code was added. It isn't explained in the PR description and there aren't any tests for it. I suspect it may have been introduced by mistake. It leads to strange handling of some edge cases. For example, consider a prop with type: [new Boolean(true)]. Obviously that's nonsense, but as of 3.4.19 (which is the first release to include that change) it behaves the same as type: Boolean. Playground example.

In the changes I've made in this PR I have not attempted to retain this behaviour. I'm now just checking for isFunction(type) && type.name, which seems to be all we need in practice.

My code does duplicate this check, once for array types and then again for non-arrays. I did try to remove the duplication, but it just ended up adding extra bytes to the build, or making it slower, or both. To see an example of one of my attempts, take a look at the first commit on this PR, 3f37cf7. That does remove the duplication, but at the expense of being slower. I also felt that version was slightly more difficult to understand for future contributors.


I've made a change to NormalizedProp to remove null from the type. From what I can tell, the null was needed when that type was first introduced, but other code has changed since, so it can now be removed. Removing the null allowed me to remove the if (prop) check in normalizePropsOptions. While this doesn't really impact the performance, it did seem to be redundant and shaves a few bytes off the bundle.


The main change is to introduce a single for loop over the type array. The type is then checked directly against the strings 'Boolean' and 'String', rather than using multiple calls to getType to get those strings. The getType function is no longer used here, and is now only used in dev code, allowing it be to treeshaken.

I experimented with using other types of loop, such as some and for/of, but ultimately a basic for seemed to give the best performance. In particular, using for/of seemed slightly slower in Firefox. The superior performance of a basic for is maybe no great surprise, but nevertheless I did try to confirm that it really was best when working with short arrays in current browsers.


Slight tangent...

I find the Brotli sizes to be a bit erratic. They seem to jump around almost randomly. For example, at the time of writing, the Brotli/overall value given in the size report is +2. But if I swap round these two lines to be in the opposite order, it jumps to -41:

let shouldCast = false
let shouldCastTrue = true

I gave up chasing the Brotli size and chose to focus on Size and Gzip instead, as they seemed like a more reliable way to judge progress.

Copy link

Size Report

Bundles

File Size Gzip Brotli
runtime-dom.global.prod.js 89.9 kB (-116 B) 34.4 kB (-50 B) 31.1 kB (-4 B)
vue.global.prod.js 146 kB (-116 B) 53.9 kB (-25 B) 48 kB (-41 B)

Usages

Name Size Gzip Brotli
createApp 49.6 kB (-116 B) 19.5 kB (-47 B) 17.8 kB (-48 B)
createSSRApp 53.2 kB (-116 B) 20.9 kB (-53 B) 19.1 kB (-44 B)
defineCustomElement 51.9 kB (-116 B) 20.2 kB (-41 B) 18.5 kB (-36 B)
overall 63.1 kB (-116 B) 24.5 kB (-49 B) 22.3 kB (+2 B)

@edison1105
Copy link
Member

/ecosystem-ci run

@vue-bot
Copy link
Contributor

vue-bot commented Jul 22, 2024

📝 Ran ecosystem CI: Open

suite result latest scheduled
language-tools failure failure
nuxt success success
pinia success success
primevue success success
quasar success success
radix-vue success success
router success success
test-utils success success
vant success success
vite-plugin-vue success success
vitepress success success
vue-i18n success success
vue-macros success success
vuetify success success
vueuse success success
vue-simple-compiler success success

@yyx990803 yyx990803 merged commit 5680142 into vuejs:main Jul 29, 2024
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants