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(core): use startsWith instead of indexOf 0 #989

Merged
merged 4 commits into from
Apr 20, 2020

Conversation

aztalbot
Copy link
Contributor

In a few places, someString.indexOf(somePrefix) === 0 is used to check if a string starts with some prefix. Using startsWith instead should enable the check to short-circuit in the negative case (indexOf will scan the whole string in the negative case); it also has clearer intent. In one case, checking if something was a v-model listener, the logic was checking existence of a string, but it should always be a prefix, so startsWith seems to make more sense.

@rigor789
Copy link
Contributor

Here's a perf test for both - not really representative of real-world cases, as you'd never have 100s of characters in a string like this.

In positive cases, indexOf is faster, but for longer strings startsWith seems faster.

@aztalbot
Copy link
Contributor Author

aztalbot commented Apr 18, 2020

@rigor789 thanks for that. I should have included the benchmark I had, but I actually like the tool you linked better. My assumption is that in these cases, there will most often be small strings that don't start with the prefix. So here is a modified benchmark where startsWith comes out faster. The reason I make that assumption is because in places like emit, library components tend to emit lots of events, most of which I wouldn't expect to be update:.

@rigor789
Copy link
Contributor

I'm always a little skeptical with perf tests - both of our benchmarks can go either way between runs. But for the most common strings, it does seem like startsWith is indeed faster!

@posva
Copy link
Member

posva commented Apr 18, 2020

startsWith is not supported on IE though

@rigor789
Copy link
Contributor

@posva looks like it's being used in runtime-dom:
https://github.com/vuejs/vue-next/blob/9e48c51f0707bc881304e592d52aedc5faa20526/packages/runtime-dom/src/modules/style.ts#L29

There are a few more places, but they are mostly in compiler, so less of a concern:
https://github.com/vuejs/vue-next/search?q=startsWith&unscoped_q=startsWith

@aztalbot
Copy link
Contributor Author

aztalbot commented Apr 18, 2020

Right, I saw it being used in runtime-dom, so I thought it was ok to use. I also thought the planned support for IE11, mostly due to Proxies, would be via a build-time flag, so poly-filling for IE support should all only need to happen when that flag is set. Maybe startsWith isn't an important enough change to be included there, though?


To argue in favor of using startsWith and including a small polyfill in the IE11 build, I would say startsWith is the more correct algorithm/tool for the handful of places where Vue needs to check for prefixes (in all cases, it's bound to the size of the prefix, not the size of the target string). In perf-sensitive areas, that seems fairly important.

@yyx990803
Copy link
Member

Good discussions here. I originally was thinking to use indexOf everywhere in runtime packages so that we don't need to polyfill it in IE, but I think that has a few drawbacks, namely slightly worse performance in modern engines and a constraint that is somewhat cumbersome to enforce via tooling (I tried to have different TS lib configs in different packages but that ended up to be too much of a hassle). So I think the easier way is to just polyfill it in IE.

@yyx990803 yyx990803 merged commit 054ccec into vuejs:master Apr 20, 2020
pikax pushed a commit to pikax/vue-next that referenced this pull request Apr 29, 2020
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