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): memoize getType #9739

Closed
wants to merge 1 commit into from

Conversation

KaelWD
Copy link
Contributor

@KaelWD KaelWD commented Dec 2, 2023

This function takes ~10-15% of the time when rendering a large number of components with lots of defined props.

Side note, is there a reason normalizePropsOptions is called for every vnode instead of just once in defineComponent?

Copy link

github-actions bot commented Dec 2, 2023

Size Report

Bundles

File Size Gzip Brotli
runtime-dom.global.prod.js 86.8 kB (+70 B) 33 kB (+12 B) 29.8 kB (+18 B)
vue.global.prod.js 133 kB (+70 B) 49.8 kB (+16 B) 44.7 kB (+16 B)

Usages

Name Size Gzip Brotli
createApp 48.3 kB (+70 B) 19 kB (+16 B) 17.3 kB (-36 B)
createSSRApp 51.5 kB (+70 B) 20.3 kB (+22 B) 18.5 kB (+35 B)
defineCustomElement 50.7 kB (+70 B) 19.8 kB (+29 B) 18.1 kB (+43 B)
overall 61.7 kB (+70 B) 23.8 kB (+20 B) 21.7 kB (+50 B)

@yyx990803
Copy link
Member

Do you have an example repro for me to further investigate this?

Side note, is there a reason normalizePropsOptions is called for every vnode instead of just once in defineComponent?

Even for the same component, normalizePropsOptions may have different result based on the app context the component is rendered in due to possible global mixins. This is kind of a Options API legacy issue. But we do cache the resolved props for per app+component combination, so I'm not sure how this could've possibly led to performance issues.

@pikax pikax added need more info Further information is requested 🧹 p1-chore Priority 1: this doesn't change code behavior. labels Dec 11, 2023
@skirtles-code
Copy link
Contributor

I did a bit of investigation into this myself. This was the example I came up with:

  1. Start the Performance profiler in Chrome.
  2. Click the button 20 times.
  3. Stop the profiler.

getType does appear prominently in the Bottom-Up section of the profile. It's a pretty contrived example, but I was still a bit surprised by how much time was spent in getType.

I tried running a few benchmarks. Reading the name property of a function is a bit slow, maybe 2 to 3 times slower than reading a normal property. But that alone doesn't really explain it.

In my example, the main problem seems to be just that the function gets called so many times. Each button click calls it about 640,000 times.

There are 1000 components in my example, each with 160 types, so it needs to call getType 160,000 times. But that then gets multiplied by 4 because:

  1. getTypeIndex gets called twice, for String and Boolean.
  2. Each comparison in isSameType calls getType twice.

So 320,000 of those calls to getType are just a complicated way of getting the strings 'String' and 'Boolean'.

The caching in this PR didn't seem to help very much, at least in my testing. The underlying problem remains, that the function is being called far more than is necessary. From looking at how booleanIndex and stringIndex are used in normalizePropsOptions, I suspect that could be achieved much more efficiently.

But that's as far as I've got for now. I haven't tried experimenting with potential improvements. Before taking it further, I'd be curious to know whether my example is anywhere close to what @KaelWD had in mind.

@mefcorvi
Copy link
Contributor

mefcorvi commented Aug 5, 2024

@yyx990803 I wonder, isn't it possible to just use ctor.name in getType when it is available? I believe the most expensive part of the code is ctor.toString().match(/^\s*(function|class) (\w+)/). If we can avoid this, we can eliminate the need for caching. I'm also not sure about cases when getType is called for objects from different iframes or VMs. Do they really exist?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🧹 p1-chore Priority 1: this doesn't change code behavior. need more info Further information is requested
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants