Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

Performance: Eliminate Regex overhead in AvoidTrailingWhitespace -> Speedup of 5% (PowerShell 5.1) or 2.5 % (PowerShell 7.1-preview.2)#1465

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
bergmeister merged 7 commits intoPowerShell:masterfrombergmeister:perf/regex
Apr 28, 2020

Conversation

@bergmeister
Copy link
Collaborator

@bergmeisterbergmeister commentedApr 27, 2020
edited
Loading

PR Summary

Whitespace ignoring diff makes it clearer. This was the most expensive script analysis rule when being run in warm mode and also easy to fix :-)
It also shows the performance improvements in .Net Core 5

PR Checklist

@bergmeisterbergmeister changed the titlePerformance: Eliminate Regex overhead in AvoidTrailingWhitespace -> Speedup of 5% (PowerShell 5.1) or 2.5 % (PowerShell 7)Performance: Eliminate Regex overhead in AvoidTrailingWhitespace -> Speedup of 5% (PowerShell 5.1) or 2.5 % (PowerShell 7.1-preview.2)Apr 27, 2020
Copy link
Contributor

@rjmholtrjmholt left a comment

Choose a reason for hiding this comment

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

If we use regexes anywhere else in the codebase, we could probably save some performance by just making the regex static and constructing it withRegexOptions.Compile

));
continue;
}
if(line[line.Length-1]!=' '&&
Copy link
Contributor

Choose a reason for hiding this comment

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

Would this be better aschar.IsWhiteSpace(line[line.Length - 1])?

Copy link
CollaboratorAuthor

@bergmeisterbergmeisterApr 27, 2020
edited
Loading

Choose a reason for hiding this comment

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

@rjmholt because of

  • readablity
  • perfomance
  • covering the variety of unicode chars? from the docshere, it would probably be good but what about theUnicodeCategory.LineSeparator char? I don't have much Unicode experience to make a judgement call here tbh if this list includes too much or not
    image

Copy link
Contributor

Choose a reason for hiding this comment

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

My thinking here is actually just thatPowerShell uses that API to see whitespace.

Given how we split the string already, it's possibly dangerous to go by unicode whitespace, but possibly not...

I suspect that really this won't make much difference; leaving non-ASCII whitespace at the ends of lines isn't something I can imagine being an issue for anyone really.

Copy link
CollaboratorAuthor

Choose a reason for hiding this comment

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

Ok, so that sounds more like a tendency to useIsWhiteSpace? I'd be OK with that, you are right that the impact is probably quite low, especially since this rules is not enabled by default for vs-code users.

vardiagnosticRecords=newList<DiagnosticRecord>();

string[]lines=Regex.Split(ast.Extent.Text,@"\r?\n");
string[]lines=ast.Extent.Text.Split(new[]{"\r\n","\r","\n"},StringSplitOptions.None);
Copy link
Contributor

Choose a reason for hiding this comment

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

This makes me wonder: if we're just trying to find the extents of trailing whitespace, there's no need to split the string at all; we should just read through ourselves without allocating all these strings... But too much burden for this PR!

Copy link
CollaboratorAuthor

@bergmeisterbergmeisterApr 27, 2020
edited
Loading

Choose a reason for hiding this comment

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

Hmm, yh, I hear what you say, I guess for perf what counts is the 80-20 rule :-) Technically speakingstring.IndexOf would probably the fastest way of finding the indices where\s\r or \s\n occurs....
I'm aware of lot's of other small micro optimisations that one can make and even tried some but they didn't have a measurable outcome. Therefore I am focussed on just fixing what gives at least a measurable return.

rjmholt reacted with thumbs up emoji
bergmeisterand others added2 commitsApril 27, 2020 22:05
Co-Authored-By: Robert Holt <rjmholt@gmail.com>
@bergmeisterbergmeister merged commit6fa29cb intoPowerShell:masterApr 28, 2020
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@JamesWTruherJamesWTruherAwaiting requested review from JamesWTruher

1 more reviewer

@rjmholtrjmholtrjmholt approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@bergmeister@rjmholt

[8]ページ先頭

©2009-2025 Movatter.jp