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

dev: processor optimizations #5316

Merged
merged 6 commits into from
Jan 14, 2025
Merged

dev: processor optimizations #5316

merged 6 commits into from
Jan 14, 2025

Conversation

ldez
Copy link
Member

@ldez ldez commented Jan 11, 2025

I was working on the new exclusion configuration #5297, and I found some "optimizations".

  • IdentifierMarker processor update:
    • removes unused regexp
    • checks the match only if the issue is from a linter with the associated pattern instead of trying to match all regex on all issue texts.
    • adds one test by pattern.
    • Note: I'm unsure if keeping/removing this processor in the future is a good idea.
  • new PathAbsoluter processor:
    • the absolute paths were obtained by the Cgo processor and the FilenameUnadjuster then now the PathAbsoluter does that once.
    • in theory, all the paths from analysis.Diagnostic are absolute.
    • FYI the processor PathPrettifier (it already exists) converts absolute path to relative (related to the different elements but mainly to exclusions)

This is not only "optimizations", the PathAbsoluter is something I will use with the new exclusion system.

I found another optimization: the usage of FileCache inside misspell.
The FileCache is used by SourceCode, ExcludesRules, severity, Fixer processors, and misspell.
The FileCache is loaded with file content and the keys are paths, but misspell is the only one that uses absolute paths to load/read data from/to the cache.
So the cache is filled but never used.
This allows to reduce the memory usage (ex: on k8s this 50MiB)

These are minor optimizations: a few seconds, but they can impact large projects.

To avoid creating a PR for exclusions with some off-topics, and as those elements are standalone, I prefer to create a PR with just these processors.

@ldez ldez added area: processors enhancement New feature or improvement labels Jan 11, 2025
@ldez ldez added this to the next milestone Jan 11, 2025
@ldez ldez force-pushed the feat/processors branch 2 times, most recently from 72b2aba to 3215445 Compare January 11, 2025 15:23
@ldez ldez changed the title dev: PathAbsoluter and IdentifierMarker processors dev: processor optimizations Jan 12, 2025
@ldez ldez requested a review from bombsimon January 12, 2025 14:55
@ldez ldez force-pushed the feat/processors branch 2 times, most recently from 4642d9e to 2e4d3d4 Compare January 12, 2025 15:10
Copy link
Member

@bombsimon bombsimon left a comment

Choose a reason for hiding this comment

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

Nice!

Re IdentifierMarker, I think it's a bit weird. It feels like it's from early days where the linters and their diagnostics was more controlled. Surely I'd like to have identifiers within backtiks just as much as the next person, but having a processor doing search replace on hand written regexes for each linter feels a bit unnecessary and hard to maintain. I'm sure there are more identifiers out there we don't replace, or replacements that are removed from the upstream linters anyways.

@ldez
Copy link
Member Author

ldez commented Jan 14, 2025

I agree, we will not remove it for now, I want to avoid "breaking changes" but we will not add new regexp unless there is a real need.

@ldez ldez merged commit 6d29861 into golangci:master Jan 14, 2025
15 checks passed
@ldez ldez deleted the feat/processors branch January 14, 2025 21:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: processors enhancement New feature or improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants