Repository navigation
optimizations for KMP - #492
Conversation
FardaleM
left a comment
FardaleM
left a comment
There was a problem hiding this comment.
When giving an empty needle, Find.compile and Find.rcompile now raise Invalid_argument.
This behaviour is not yet documented in the Split module for Split.{gen,list,seq,iter}(_copy)? functions. Also, the documentation of CCStringLabel need the same changes as CCString.
Maybe this should be in the release note as breaking change. For most functions, it is more a bug fix, but for String.find and String.mem for example, the previous behaviour (0 and true) were kind of making sense and could be legitimately used.
The main concern I have is with respect to the lazy construction of the failure table and if it works when sharing a pattern in multiple domain. But I don't know enough how multicore works to tell.
Otherwise, nice improvement! It was fun to review. Where does this idea come from?
| if fail_offset >= 0 then ( | ||
| assert (fail_offset < !j); | ||
| i := !i + !j - fail_offset; | ||
| j := fail_offset | ||
| ) else ( | ||
| j := 0; | ||
| incr i | ||
| ) |
There was a problem hiding this comment.
| if fail_offset >= 0 then ( | |
| assert (fail_offset < !j); | |
| i := !i + !j - fail_offset; | |
| j := fail_offset | |
| ) else ( | |
| j := 0; | |
| incr i | |
| ) | |
| assert (fail_offset < !j); | |
| i := !i + !j - fail_offset; | |
| if fail_offset >= 0 then j := fail_offset; |
If I understand the algorithm correctly, we have the following:
fail_offset < 0 iff fail_offset = -1 iff j = 0.
Therefore, in the case fail_offset <0:
i := !i + !j - fail_offset correspond to i := !i + 0 - (-1) which is incr i
j := 0 is not useful, as j is already 0.
| target instead of re-comparing it and failing again *) | ||
| for k = 1 to len - 1 do | ||
| let b = failure.(k) in | ||
| if b >= 0 && CCChar.equal pat.[k] pat.[b] then |
There was a problem hiding this comment.
| if b >= 0 && CCChar.equal pat.[k] pat.[b] then | |
| if CCChar.equal pat.[k] pat.[b] then |
b >= 0 should always be true when k>0.
| if fail_offset >= 0 then ( | ||
| assert (fail_offset < !j); | ||
| (* follow the failure link *) | ||
| i := !i + !j - fail_offset; | ||
| j := fail_offset | ||
| ) else ( | ||
| (* beginning of pattern *) | ||
| j := 0; | ||
| incr i | ||
| (* no usable border: advance past the mismatch *) | ||
| i := !i + !j + 1; | ||
| j := 0 | ||
| ) |
There was a problem hiding this comment.
Same as in the benchmark file. This can be simplied.
| i := !i + !j + 1; | ||
| j := 0 | ||
| ) | ||
| ) |
There was a problem hiding this comment.
The same simplification as in run_benchs.ml is possible here.
| if fail_offset >= 0 then ( | ||
| assert (fail_offset < !j); | ||
| (* follow the failure link *) | ||
| i := !i + !j - fail_offset; | ||
| j := fail_offset | ||
| ) else ( | ||
| (* beginning of pattern *) | ||
| j := 0; | ||
| incr i | ||
| (* no usable border: advance past the mismatch *) | ||
| i := !i + !j + 1; | ||
| j := 0 | ||
| ) |
try to get closer to the behavior of the new String functions in OCaml 5.6
d8fb5a5 to
55a26ba
Compare
…laces `gen` isn't thread safe here, but otherwise we make sure to use `compile` and not `compile_lazy` when intermediate state is reachable in the result (as opposed as used in a transient computation that yields a proper value as result).
2 participants
alongside benchmarks.