Implement compression of DFA states. #226
Conversation
|
Thanks! This was assigned to @killercup... |
| /// is a zero-width assertion. | ||
| flags: StateFlags, | ||
| } | ||
| struct State{data: Box<[u8]>} |
There was a problem hiding this comment.
Could you please use the same style of formatting as the rest of the code? Probably struct State(Box<[u8]>) would be nice here.
|
OK, I have a few nits but this otherwise looks good! Thank you. In the future, could you please get in touch with me or make a comment on the issue tracker before working on a non-trivial PR? |
|
Oh, yeah, I was working on that. But that's okay, your code is basically what I have written as well and then some more. Just FYI, I originally had the idea to represent the two different sizes (i8 and i32) with a postfixed bit on the first byte, to allow i7 in one byte and i31 in 4 bytes, but I'm sure that while maybe looking more clever it would've been more error prone and probably slower to read in practice. Now that I think about it, InstPtr is an u32, right? Can we correctly represent all possible deltas with i32s? (Or maybe I'm just missing something, it is pretty early here :)) |
Do you mean encode every delta as an |
|
Thanks @SeanRBurton! |
I think if the performance of reading these instruction pointers becomes a factor, then the DFA has already lost. :-) (One can only hope it will realize it, quit and fall back to the slow-but-faster-in-this-case Pike VM.) |
|
RE: #226 (comment) I guess he means that if a delta > i32::MAX then we can't represent it in 4 bytes. Maybe we should just say that insts.len() has to be <= i32::MAX to use the dfa. |
|
I was only thinking about the upper bound, where this uses 4 u8's to build a i32. It's probably irrelevant in praxis, but since InstrPtrs are u32, there could in theory be a jump from u32::MIN to u32::MAX which the i32 delta can't represent. (Maybe just add a debug_assert? Are there overflow checks?) Oh, and BTW, are there any benchmarks? Andrew mentioned in the original issue this could improve the class negation benchmark (IIRC).
|
Ah, yeah, we only use
I haven't run them yet, but I will before the next release to make sure there are no regressions. It will only impact the class negation benchmark if it brings memory usage down from 4-5MB or so to under 2MB. (When I last thought about, I didn't think this optimization was capable to doing that much.) |
This slightly modifies the implementation in #226 to use variable width integers. This saves a touch more space with regexes with huge alternations spanning larger-than-127 instructions (typically from Unicode character classes). This also adjusts `approximate_size`, which is the actual gatekeeper behind deciding whether the DFA has filled its cache or not. The approximation is now reduced slightly to account for the space savings. The variable integer encoding used is from Protocol Buffers, documented here: https://developers.google.com/protocol-buffers/docs/encoding#varints
This slightly modifies the implementation in rust-lang#226 to use variable width integers. This saves a touch more space with regexes with huge alternations spanning larger-than-127 instructions (typically from Unicode character classes). This also adjusts `approximate_size`, which is the actual gatekeeper behind deciding whether the DFA has filled its cache or not. The approximation is now reduced slightly to account for the space savings. The variable integer encoding used is from Protocol Buffers, documented here: https://developers.google.com/protocol-buffers/docs/encoding#varints
Implements the suggestion from issue #199.