deps: remove thread_local dependency #749
Merged
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
The quickcheck update seems to have sussed out a bug in our DFA logic regarding the encoding of NFA state IDs. But the bug seems unlikely to occur in real code, so we massage the test data for now until the lazy DFA gets moved into regex-automata.
It looks like it blows the default regex size limit at the moment.
This removes extraneous commentary that uses the word 'unsafe'. This makes it easier to grep for usages of meaningful 'unsafe' in the code.
This commit removes the thread_local dependency (even as an optional dependency) and replaces it with a more purpose driven memory pool. The comments in src/pool.rs explain this in more detail, but the short story is that thread_local seems to be at the root of some memory leaks happening in certain usage scenarios. The great thing about thread_local though is how fast it is. Using a simple Mutex<Vec<T>> is easily at least twice as slow. We work around that a bit by coding a simplistic fast path for the "owner" of a pool. This does require one new use of `unsafe`, of which we extensively document. This now makes the 'perf-cache' feature a no-op. We of course retain it for compatibility purposes (and perhaps it will be used again in the future), but for now, we always use the same pool. As for benchmarks, it is likely that *some* cases will get a hair slower. But there shouldn't be any dramatic difference. A careful review of micro-benchmarks in addition to more holistic (albeit ad hoc) benchmarks via ripgrep seems to confirm this. Now that we have more explicit control over the memory pool, we also clean stuff up with repsect to RefUnwindSafe. Fixes #362, Fixes #576 Ref BurntSushi/rure-go#3
|
This PR is on crates.io in |
This was referenced Mar 12, 2021
This was referenced Mar 15, 2021
This was referenced Apr 8, 2021
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.
Explanation is in the commit log, and more details are in
src/pool.rsin this patch. The short summary is that it seems likethread_localis the underlying cause of leaks in some usage scenarios, so we replace it with a simpler but more constrained memory pool. We also take this opportunity to iron out ourUnwindSafeandRefUnwindSafeAPI commitments. That is, all the variousRegextypes now impl both traits and this is now tracked in tests.Fixes #362, Fixes #576
Ref BurntSushi/rure-go#3