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

perf: improve dfa matching (with tweaks) #540

Merged
merged 8 commits into from
Dec 1, 2018
Merged

perf: improve dfa matching (with tweaks) #540

merged 8 commits into from
Dec 1, 2018

Conversation

BurntSushi
Copy link
Member

@BurntSushi BurntSushi commented Dec 1, 2018

This PR is a riff on @Marwes' excellent work in #511. For the most part, this only contains tweaks in order to get it into a mergeable PR:

  • We move forward with the MSRV a bit more aggressively, and bump to 1.24.1. We justify this by virtue of the fact that lazy_static is now also at 1.24.1, and is a transitive dependency of regex (via thread_local).
  • A large part of the diff consisted of irrelevant style changes. This made the code harder to review and was generally a huge pain to undo. This PR adds a rustfmt config to this repo that explicitly disables rustfmt in an attempt to prevent this from happening in the future.
  • There were several pieces that were undocumented, and the state map's API has been improved and lifted out of a submodule.
  • The optimization that avoided cloning a state by reading from the inst ptr scratch space was removed. I'm not in principle against this, but the way it was done violated abstraction too much for my taste.
  • Tweaked the heap size calculation. I think it was double-counting the Arcs.

And I think that's it! The rest looked great! Thanks @Marwes!

Closes #511

Markus Westerlind and others added 8 commits November 30, 2018 21:02
Shaves of a few percent on dfa heavy regexes such as `RegexSet` use.
Avoids frequent re-allocations while the instructions are pushed to it.
This was motivated in part for using `From<&[T]> for Arc<T>`, which
technically only requires 1.21.0. However, lazy_static 1.2, which is a
dependency of regex, now requires 1.24.1, so we might as well ride the
wave.
rustfmt is not something I'm willing to adopt at this time, but
contributors are still trying to use it anyway. This is irksome, so we
attempt to prevent this from happening by creating a config that
explicitly disables rustfmt.
@BurntSushi BurntSushi merged commit 2ca70c9 into master Dec 1, 2018
@BurntSushi BurntSushi deleted the ag/dfa-set branch December 1, 2018 02:53
@Marwes
Copy link
Contributor

Marwes commented Dec 1, 2018

A large part of the diff consisted of irrelevant style changes. This made the code harder to review and was generally a huge pain to undo. This PR adds a rustfmt config to this repo that explicitly disables rustfmt in an attempt to prevent this from happening in the future.

Sorry about that, I try and disable rustfmt most of the time but I think in this case it got re-formatted during when fixing conflicts in a rebase :/

The optimization that avoided cloning a state by reading from the inst ptr scratch space was removed. I'm not in principle against this, but the way it was done violated abstraction too much for my taste.

That part was ugly but it is a shame it got removed, it was actually a pretty good speedup since it avoids an allocation + copy in the common case where the cache is hit (somewhere between 5-10% if I remember).

@BurntSushi
Copy link
Member Author

BurntSushi commented Dec 1, 2018

Yeah, I figured about the optimization. It just introduced too much complexity in code that is already way too complex. I've been trying to think about ways to rewrite the DFA so that it is simpler, and in particular handles look around in a more obvious way, but I don't have any great ideas yet.

With that said, another way of looking at this is that if these types of optimizations are helping, then the DFA is spending a lot of time either building itself or thrashing, which is probably never going to be that fast in the first place. So there might be better higher level optimizations available to you. But I'm not sure.

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

Successfully merging this pull request may close these issues.

2 participants