-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Fix boyer_moore_searcher with the Rytter correction #724
Conversation
Add an overflow check for pathologically small _Diff types with large patterns.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpicky style-level comments since I feel like I let people down if I don't complain about something in a PR.
* Use auto after static_cast. * Simplify mt19937 seeding because it's always 32-bit. * Stay within the chrono::duration domain.
For what it's worth: note that although Knuth mentions Rytter's correction in his list of papers (see P71 in his vita), the addendum to his reprint of this paper (as Chapter 9 of Selected Papers on Design of Algorithms) seems to suggest that there may be simpler corrections than Rytter's: (skipping a page and a bit, where the actual correction by Mehlhorn and the simplification by Dahl are described…) — sorry if this is irrelevant or already well-known; just posting here in case it's helpful. |
This makes me really appreciate the current times where naming is actually considered as important. That said the algorithm is a clear implementation of the paper. |
This might also be of interest: "On the shift-table in Boyer-Moore’s String Matching Algorithm" by Yang Wang, which discusses a different way to compute the shift-table. It also includes an improved version (by A. V. Aho) of the original algorithm. |
Thanks, @shreevatsa and @mozjag - I wasn't aware of that additional history. I've filed #727 to track investigating those algorithms later. For now, because this is silent bad codegen and VS 2019 16.7 is locking down for release, I'm going to go ahead with the Rytter correction, but the enhanced test coverage in this PR should make it easier to replace this algorithm in the future. (As long as the performance characteristics are similar, my primary desire here is for less source code, so Mehlhorn's and possibly Aho's versions could be interesting; my quick glance at Wang's algorithm makes me think that it requires two phases and more source code. However, if we find that there are significant performance differences with modern hardware/compilers, then that's worth paying significantly more source code here.) |
This fixes #713, silent bad codegen in an important C++17 feature.
The Boyer-Moore algorithm (published 1977) relies on a "delta2" table, but that paper didn't explain how to generate the table. Another paper by Knuth, Morris, and Pratt (also 1977) provided the algorithm for the delta2 table. Actually, it provided two algorithms producing tables compatible with the usage of delta2: a basic algorithm
dd
and an improved algorithmdd'
. We implemented the latter, and properly translated it from 1-based indexing to 0-based indexing.However, the published algorithm for
dd'
was incorrect! This was discovered and fixed by Rytter in 1980, which we weren't aware of until we received a bug report. While the "Rytter correction" was known in the computer science literature, I find it very curious that it isn't constantly mentioned when explaining Boyer-Moore (e.g. Wikipedia's page currently makes no mention of this).This PR applies this 40-year-old bugfix and significantly expands our test coverage.
<functional>
const
to_Shifts
. (This is thedd'
output array; I kept the name.)_Pat_size
to_Mx
to follow the naming in the published algorithms._RanIt
doesn't exist, it's_RanItPat
._Suffix_fn
to_Fx
, again following the papers. Note that in the usage below, there is a semantic change:_Suffix_fn
stored 0-based values, while_Fx
stores 1-based values. This undoes a micro-optimization (_Suffix_fn
avoided unnecessarily translating between the 0-based and 1-based domains), but with the increased usage off
in the Rytter correction, I wanted greater correspondence with the published algorithms in order to verify the implementation._Fx
can be top-levelconst
(we don't reassign/reset theunique_ptr
).unique_ptr<T[]>
usesdefault_delete<T[]>
which usesdelete[]
, so we should usenew[]
to match, not::new[]
. (This makes a difference only for pathologically fancy_Diff
types.)_Idx
to 1-based_Kx
._Suffix
to 1-based_Tx
._Idx
to 1-based_Jx
. (While the code was correct, I found it confusing that_Idx
was 0-based in other loops but 1-based in this loop.)_Temp
after verifying that this doesn't disturb the algorithm.P0220R1_searchers/test.cpp
test_boyer_moore_table2_construction()
from Rytter's paper, and other repetitive patterns where the Rytter correction produces different results. Those patterns were "made from scratch" but for the results, I just used the output of the implementation and manually verified selected answers for the AB and ABC categories against my understanding of delta2's meaning (including the unused last entry, see <functional>: Boyer-Moore's delta_2 table contains an unnecessary last entry #714; I was able to understand why it's 10 for"aaaaaaaaaa"
, 2 for"abaabaabaa"
, and why it should be 1 for"ababababab"
and"abcabcabc"
).mt19937
(for speed, instead of directly usingrandom_device
). It prints out the needle/haystack for any failures, so we don't need the seed printing/reload machinery fromP0067R5_charconv/test.cpp
. (I'm usingmt19937
for 32-bit performance, since we don't need 64-bit values.) The randomized coverage uses alphabets from[a-b]
to[a-f]
; the former finds more examples of the bug being fixed here (as it's more likely to create highly repetitive patterns). Expanding the alphabet makes repetition unlikely, which is why I stop at[a-f]
. The test does a few things to improve non-optimized debug performance (reusingneedle
andhaystack
to avoid repeated allocations, usingconst char *
to avoid iterator overhead), although I didn't pursue this to the ultimate limit (e.g.uniform_int_distribution
is somewhat costly and could be avoided at the expense of some bias). Finally, as with the other randomized coverage, I print out timing statistics if it takes a long time; on my i7-8700 it's tuned to take ~400 ms which seems reasonable (given the number of configurations, the performance of VMs, and the time needed for compilation).While this changes the behavior of a function, it is ABI-safe. This doesn't require coordinated changes across functions - the rest of the Boyer-Moore machinery is unchanged, and the layout of the delta2 table is unchanged. We're simply filling it with different values. In the event of mismatch, the linker will either pick the correct or incorrect algorithm, so this can't make things any worse.