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

Gazelle is >5x slower after #1022 merged #1026

Closed
wolfd opened this issue Apr 2, 2021 · 3 comments · Fixed by #1071
Closed

Gazelle is >5x slower after #1022 merged #1026

wolfd opened this issue Apr 2, 2021 · 3 comments · Fixed by #1071

Comments

@wolfd
Copy link
Contributor

wolfd commented Apr 2, 2021

What version of gazelle are you using?

current master (.bazelignore change #1022): d1bb564
previous commit (#1024 ): 876c759

What version of rules_go are you using?

v0.25.1

What version of Bazel are you using?

4.0.0

What operating system and processor architecture are you using?

Ubuntu Linux x86_64

What did you do?

Swapped out gazelle version in workspace and ran our gazelle tooling under both versions:

current master (.bazelignore change #1022): d1bb564

bazel run --run_under="time" //tools/gazelle

real    0m15.114s
user    0m21.834s
sys     0m0.578s

previous commit (#1024): 876c759

bazel run --run_under="time" //tools/gazelle

real    0m1.978s
user    0m2.938s
sys     0m0.385s

For what it's worth, our .bazelignore has ~50 items.

For now, I've chosen to run 876c759, but the performance regression in d1bb564 is giving me pause to updating to latest.

@wolfd wolfd changed the title Gazelle is ~10x slower after #1022 merged Gazelle is >5x slower after #1022 merged Apr 2, 2021
@wolfd
Copy link
Contributor Author

wolfd commented Apr 5, 2021

Looking a bit into this, I think d1bb564#r49128622 is causing this issue. Is there a good place to cache that information?

@danny-skydio
Copy link
Contributor

danny-skydio commented Apr 15, 2021

Dug a bit more into this, and it seems that the issue is more to do with how the # gazelle:exclude xyz feature works. Caching the parsed version of .bazelignore only marginally improved wall-clock time in my latest testing.

Some more info:

I suspect that a fix here is to have a more complicated implementation of excludes.

For .bazelignore, we could probably generate a mapping, and check if these are files or folders once upon loading. Walk could potentially look up items in that static map to exclude them, without looping over each item in the .bazelignore for every file in the source tree.

I also suspect that a similar change could be made to regular # gazelle:exclude xyz behavior, where the expression is checked to see if it has ** globs or / separators, and otherwise just restricting the exclude to the same directory. I think this would be equivalent behavior, but I'm not certain. Making this change would also fix the .bazelignore performance hit.

@jayconrod
Copy link
Contributor

Marking this as a bug. Thanks for investigating this. It sounds like the best thing to do is to improve the exclude implementation rather than roll back bazelignore support.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants