lots of little polishing #2614
Merged
lots of little polishing #2614
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Not much here. Just updating to reflect my current style and bringing the crate to the 2021 edition.
I think I already did a clean-up of this crate when I moved it to regex 1.9, so the polish here is very minor.
And use rustdoc's native intra-crate links. So much nicer.
This brings the code in line with my current style. It also inlines the dozen or so lines of code for FNV hashing instead of bringing in a micro-crate for it. Finally, it drops the dependency on regex in favor of using regex-syntax and regex-automata directly.
BurntSushi
force-pushed
the
ag/polish
branch
from
b440b99 to
d7c4277
Compare
This updates some dependencies and brings code style in line with my current practice.
In the time before, we just used a RegexSet from the regex crate. That allocated unconditionally, so there was nothing we could do and it didn't expose any APIs to reuse that memory. But now that we're using the lower level regex-automata, we can reuse a PatternSet. Ideally we would just provide a way for the caller to build a PatternSet (perhaps via an opaque type) so that we don't have to shuffle data into a PatternSet and then back into the caller's `Vec<usize>`. But this at least avoids allocating for every search.
Like previous commits, we do a bit of polishing and bring the style up to my current practice.
This is largely made possible by the addition of std::sync::OnceLock to the standard library, and the memory pool available in regex-automata.
This I believe finishes are quest to do mechanical updates to ripgrep's style, bringing it in line with my current practice (loosely speaking).
It was apparently using a format specific to a particular plugin. I did know that, but apparently the plugin is not ubiquitous or de facto standard[1]. Thus, including it I think just leads to more confusion. We definitely do not want to be in the business of bundling aliases for every conceivable plugin to different editors, so just drop it. We expose the ability to write your own format for exactly this sort of reason. [1]: #2611 (comment)
BurntSushi
force-pushed
the
ag/polish
branch
from
3d01666 to
70b0936
Compare
ripgrep does not, and likely never will, report which pattern matched. Because of that, we can dedup the patterns via just their concrete syntax without any fuss. This is somewhat of a pathological case because you don't expect the end user to pass duplicate patterns in general. But if the end user generated a list of, say, names and did not dedup them, then ripgrep could end up spending a lot of extra time on those duplicates if there are many of them. By deduping them explicitly in the application, we essentially remove their extra cost completely.
This brings in a fix for a bug I found during ad hoc benchmarking: rust-lang/regex@aa4e4c7
These seem to help when ripgrep emits a lot of output, especially when the --column flag is used.
It seems like a trifle, but if the match frequency is high enough, the allocation+formatting of line numbers (and columns and byte offsets) starts to matter. We squash that part of the profile in this commit by doing our own decimal formatting. I speculate that we get a speed-up from this by avoiding the formatting machinery and also a possible allocation. An alternative would be to use the `itoa` crate, and it is indeed marginally faster in ad hoc benchmarks, but I'm satisfied enough with this solution.
Many of these functions should be inlineable, but I'm not 100% sure that they can be inlined without these annotations. We don't want to force things, but we do try and nudge the compiler in the right direction.
I did this in the course of trying to optimize it. I don't believe I made it any faster, but the refactoring led to code that I think is more readable.
This was probably fixed in a past commit where I bumped the regex engine to 1.9 (or perhaps more precisely, regex-automata 0.3). But I didn't track it as fixed at the time. Fixes #1275
Like the previous CHANGELOG entry, this marks a bug that was fixed
likely with the introduction of regex 1.9:
$ hyperfine "rg-13.0.0 -ic '\bfoo\b \bbar\b' git-3a06386e.txt" "rg -ic '\bfoo\b \bbar\b' git-3a06386e.txt"
Benchmark 1: rg-13.0.0 -ic '\bfoo\b \bbar\b' git-3a06386e.txt
Time (mean ± σ): 1.034 s ± 0.011 s [User: 1.030 s, System: 0.004 s]
Range (min … max): 1.021 s … 1.053 s 10 runs
Benchmark 2: rg -ic '\bfoo\b \bbar\b' git-3a06386e.txt
Time (mean ± σ): 6.3 ms ± 0.3 ms [User: 4.6 ms, System: 1.6 ms]
Range (min … max): 5.6 ms … 7.3 ms 343 runs
Summary
'rg -ic '\bfoo\b \bbar\b' git-3a06386e.txt' ran
164.95 ± 7.70 times faster than 'rg-13.0.0 -ic '\bfoo\b \bbar\b' git-3a06386e.txt'
This was not fixed by making \b itself faster, but rather, by improving
inner literal extraction. In particular, if the regex doesn't have any
literals extracted, then search time can still be quite slow:
$ time rg-13.0.0 -ic '\b[a-z]{3}\b\s\b[a-z]{3}\b' git-3a06386e.txt
57538
real 0.427
user 0.423
sys 0.003
maxmem 46 MB
faults 0
$ time rg -ic '\b[a-z]{3}\b\s\b[a-z]{3}\b' git-3a06386e.txt
57538
real 0.337
user 0.333
sys 0.003
maxmem 46 MB
faults 0
But then again, so is grep, because grep doesn't benefit from any
literal optimizations either:
$ time grep -E -ic '\b[a-z]{3}\b\s\b[a-z]{3}\b' git-3a06386e.txt
62396
real 1.316
user 1.292
sys 0.007
maxmem 13 MB
faults 7
The count mismatch should probably be investigated.
Fixes #1760
This was fixed a few commits ago when we updated to regex-automata 0.4 (regex 1.10). Fixes #2623
I'm too lazy to fixup old commits.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR brings a lot of the code into line with my current style, which has slightly shifted over the years. I've also dropped dependencies where possible. For example,
globsetno longer depends on thefnvcrate, and instead just rolls the FNV hashing itself. Similarly, theregexcrate is dropped as a dependency entirely, and we instead depend onregex-automatadirectly. This leads to the interesting result that ripgrep no longer depends onregexanywhere in its tree(!), but it is still most convenient to say that it uses the regex crate when talking about the kind of regex flavor it supports.