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

improvement: improve performance #1839

Merged
merged 3 commits into from
Apr 13, 2020

Conversation

IWANABETHATGUY
Copy link
Contributor

i found a big performance issue when open a big vue project, and fix it , pass all test.

@IWANABETHATGUY
Copy link
Contributor Author

the same project, after open a few .vue file, i profile it.
current version:
Xnip2020-04-12_15-24-31
this_pr version
Xnip2020-04-12_14-59-48
current updateOffsetMapping

Xnip2020-04-12_15-25-14

pr updateOffsetMapping
Xnip2020-04-12_14-59-34

@IWANABETHATGUY
Copy link
Contributor Author

i hope this pr can be merged, so more people may benefit this improve @octref

@ktsn
Copy link
Member

ktsn commented Apr 12, 2020

If the perf bottle neck is toFiltered, can we only change it? Looks like inlining for loop for to and 'this.'.length are unnecessary?

@IWANABETHATGUY
Copy link
Contributor Author

If the perf bottle neck is toFiltered, can we only change it? Looks like inlining for loop for to and 'this.'.length are unnecessary?

Because the string literal length this. can be calculated before runtime. This will not cause extra overhead . I think that if there is no particular impact on readability, performance should be the most important consideration.

@ktsn
Copy link
Member

ktsn commented Apr 12, 2020

According to your screen shot, there are no particular differences between them.

  • for loop
    • 6.9ms vs. 7.6ms
  • 'this.'.length
    • 1.4ms vs. 1.3ms

So I think we should keep them. We intentionally wrote as 'this.'.length to indicate what the offset is for.

@IWANABETHATGUY
Copy link
Contributor Author

ok, i will do that

Co-Authored-By: Katashin <ktsn55@gmail.com>
@IWANABETHATGUY
Copy link
Contributor Author

@ktsn complete that

Copy link
Member

@ktsn ktsn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

@ktsn ktsn merged commit 7be7693 into vuejs:master Apr 13, 2020
ktsn added a commit that referenced this pull request Apr 13, 2020
@IWANABETHATGUY
Copy link
Contributor Author

@ktsn please don't publish now, i found this pr could cause .vue file outline issue, i will check that out

@IWANABETHATGUY
Copy link
Contributor Author

@ktsn i'm sorry, it's not a bug, it's my vscode config issue, it's ok now .👍

@IWANABETHATGUY IWANABETHATGUY deleted the performance-big-improvement branch May 15, 2020 07:59
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.

2 participants