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: preinitialize typeCheck RegExp #10990

Merged
merged 1 commit into from Mar 30, 2021
Merged

Conversation

SebastianSpeitel
Copy link
Contributor

@SebastianSpeitel SebastianSpeitel commented Jan 7, 2020

What kind of change does this PR introduce? (check at least one)

  • Bugfix
  • Feature
  • Code style update
  • Refactor
  • Build-related changes
  • Other, please describe: Performance improvement

Does this PR introduce a breaking change? (check one)

  • Yes
  • No

If yes, please describe the impact and migration path for existing applications:

The PR fulfills these requirements:

If adding a new feature, the PR's description includes:

  • A convincing reason for adding this feature (to avoid wasting your time, it's best to open a suggestion issue first and wait for approval before working on it)

Other information:

After looking at the performance of one of my apps, I noticed, the function, that took the most time summed up was getType() , defined in [1] taking 200ms. I noticed the regular expression getting created for each call and measured again including the proposed improvement, which took the time down to 150ms. To make sure I changed it back to confirm and it again timed at 200ms.

[1]

vue/src/core/util/props.js

Lines 182 to 185 in b07087d

function getType (fn) {
const match = fn && fn.toString().match(/^\s*function (\w+)/)
return match ? match[1] : ''
}

@SebastianSpeitel SebastianSpeitel changed the title Predefine typeCheck RegExp perf: preconstruct typeCheck RegExp Jan 7, 2020
@SebastianSpeitel SebastianSpeitel changed the title perf: preconstruct typeCheck RegExp perf: preinitialize typeCheck RegExp Jan 7, 2020
@SebastianSpeitel
Copy link
Contributor Author

Maybe further improvements could be made by removing the regular expression completely and using standard string manipulation methods.

@SebastianSpeitel
Copy link
Contributor Author

Is there anything else, that needs to be done before merging? @posva

@posva
Copy link
Member

posva commented Jan 14, 2020

@SebastianSpeitel You don't need to do anything else, thank you :)

@SebastianSpeitel
Copy link
Contributor Author

I made this PR 2 months ago. Is there a reasons for this taking so long?

@posva posva removed the semver:minor label Sep 4, 2020
@posva posva added this to In Review in 2.6.13 Feb 24, 2021
@posva posva moved this from In Review to Reviewed once, needs another review in 2.6.13 Mar 9, 2021
@posva posva merged commit 2488a6a into vuejs:dev Mar 30, 2021
@posva posva moved this from Reviewed once, needs another review to Done in 2.6.13 Mar 30, 2021
@lefreet
Copy link

lefreet commented May 26, 2021

@SebastianSpeitel cached it may be better ?

  const getType = cached(function getType (fn) {
    var match = fn && fn.toString().match(/^\s*function (\w+)/);
    return match ? match[1] : ''
  })

@SebastianSpeitel
Copy link
Contributor Author

@SebastianSpeitel cached it may be better ?

  const getType = cached(function getType (fn) {
    var match = fn && fn.toString().match(/^\s*function (\w+)/);
    return match ? match[1] : ''
  })

Both could be optimal.
Compile the regex only once and compute the type of each object only once.

@lefreet
Copy link

lefreet commented Jun 7, 2021

but the fn.toString is a expense also, for every function @SebastianSpeitel sorry my later comment

@tophf
Copy link

tophf commented Oct 14, 2021

Why not just type.name instead of the entire getType?

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

Successfully merging this pull request may close these issues.

None yet

5 participants