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

Improve performance of package comment regex #1799

Merged
merged 2 commits into from May 25, 2018

Conversation

2 participants
@kalafut
Contributor

kalafut commented Apr 16, 2018

By anchoring to the start of the document (since that's the only place
a package comment will appear), the number of false matches is greatly
reduced in some files.

Fixes #1747

Improve performance of package comment regex
By anchoring to the start of the document (since that's the only place
a package comment will appear), the number of false matches is greatly
reduced in some files.

Fixes #1747
@Carpetsmoker

This comment has been minimized.

Collaborator

Carpetsmoker commented Apr 16, 2018

A package comment is not always at the start of the document; it can be preceded with one or more other comment blocks.

For example this file will no longer work correct:

// +build foo

/*
Copyright ...
*/

// Package main should be folded
// fold this.
package main

func main() {
}

I'm not sure if there is a way to make this significantly faster; maybe using \%^.* is faster, or maybe not.

Screenshots

Master:
2018-04-16-052451_623x379_scrot

This PR:
2018-04-16-052432_513x368_scrot

@kalafut

This comment has been minimized.

Contributor

kalafut commented Apr 16, 2018

OK, I see what you mean. Let me see if I can adjust this.

@kalafut

This comment has been minimized.

Contributor

kalafut commented Apr 21, 2018

I played around with a number of regex that mostly handled these cases, but they became uncomfortably complex. Taking a step back, the real performance problems stem from the /* ... */ matching, since they can be started with any /* and end up searching the whole rest of the file. So a completely different approach to this PR is simply to replace:

start=/\v\/\*.*\n(.*\n)*\s*\*\/\npackage/ with
start=/\v^\s*\/\*.*\n(.*\n)*\s*\*\/\npackage/

This start of line anchor helps a lot because it prevents quoted /* from starting new searches, and is (I think?) still valid for package comments.

Note that if there are block (/* .. */) comments (like a license) and then a block commented package comment, the license+package get folded together, like you were just mentioning. But that behavior is present even in the released version, and line comment style is more common anyways.

If you like this approach I'll update that PR with it.

@fatih fatih requested a review from Carpetsmoker May 8, 2018

@Carpetsmoker

This comment has been minimized.

Collaborator

Carpetsmoker commented May 25, 2018

Seems like a good solution 👍 Thanks!

@Carpetsmoker Carpetsmoker merged commit b3a1a1c into fatih:master May 25, 2018

2 of 3 checks passed

codecov/patch 0% of diff hit (target 30.64%)
Details
codecov/project 30.64% remains the same compared to 7491209
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment