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

Avoid re-compiling rules #20

Open
skeggse opened this issue Jul 21, 2021 · 2 comments
Open

Avoid re-compiling rules #20

skeggse opened this issue Jul 21, 2021 · 2 comments

Comments

@skeggse
Copy link

skeggse commented Jul 21, 2021

I observed, while looking into #19, that due to the current implementation we're actually re-compiling every rule for every file. Updating the range in matchIgnoreRule to be by-reference improves performance on large repositories by a factor of ~3.

@apparentlymart
Copy link
Contributor

I independently noticed the same thing while working on ccbbeee and attempted to improve it, but since performace wasn't my main motivation in that commit I didn't spend much time studying the impact of the change I attempted there.

Looking back at it again with fresh eyes today I suspect I may not have actually fully addressed it: @skeggse's note about passing by reference instead of by value makes me notice that I seem to have made it just ineffectually precompile copies of the default rules, rather than the actual values that end up in the default ruleset:

var defaultExclusions = []rule{
{
val: strings.Join([]string{"**", ".git", "**"}, string(os.PathSeparator)),
excluded: false,
},
{
val: strings.Join([]string{"**", ".terraform", "**"}, string(os.PathSeparator)),
excluded: false,
},
{
val: strings.Join([]string{"**", ".terraform", "modules", "**"}, string(os.PathSeparator)),
excluded: true,
},
}
func init() {
// We'll precompile all of the default rules at initialization, so we
// don't need to recompile them every time we encounter a package that
// doesn't have any rules (the common case).
for _, r := range defaultExclusions {
err := r.compile()
if err != nil {
panic(fmt.Sprintf("invalid default rule %q: %s", r.val, err))
}
}
}

A small change that might make it effectual is to refer directly to the array elements by index, rather than working on the copies that the for ... range is implicitly making:

	for i := range defaultExclusions {
		err := defaultExclusions[i].compile()
		if err != nil {
			panic(fmt.Sprintf("invalid default rule %q: %s", r.val, err))
		}
	}

The default match object includes a slice value covering the same backing array, so I expect that a change like the above would make it effective, but I've not yet tested.

@skeggse
Copy link
Author

skeggse commented Dec 20, 2023

@brandonc i don't suppose you looked at this issue while you were working on the directory issue?

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

No branches or pull requests

2 participants