Experiment with changes to the BiMap library#173
Closed
andir wants to merge 12 commits intopython-grimp:masterfrom
Closed
Experiment with changes to the BiMap library#173andir wants to merge 12 commits intopython-grimp:masterfrom
andir wants to merge 12 commits intopython-grimp:masterfrom
Conversation
This is the main graph implemented in Rust - but it isn't plumbed into Python yet.
This adds a wrapper around the Rust-graph in lib.rs which handles the interface between Python and Rust. At the same time, it changes the Python ImportGraph so it uses Rust instead.
This is faster, but not cryptographically safe. That shouldn't matter for our purposes.
CodSpeed Performance ReportMerging #173 will degrade performances by 51.02%Comparing Summary
Benchmarks breakdown
|
This is using a bit of state to store the potential expensive recomputation of get_modules between invocations. The current fallback, in case of concurrent access, is to recalcuate the data instead of waiting for the lock to be release.
This allows passing a string reference rather than an owned copy. This can, in some cases, removes additional copies that the compiler might not get rid of.
We don't need that anymore as the Module::new invocation will take care of that. Since the String will be stored in an Arc<> that hopefully leads to the String instance being done in-place.
This reduces the amount of benchmarking noise that might stem from noise in filesystem access time.
Most of the time the result should be cached. Unfortunately we are required to keep it in a Sync structure (thus a Mutex + Arc is being used). To workaround this the MutexGuard that we acquire during the function call is moved into another struct that supports a std::ops::deref-alike function which returns either a reference to an owned copy (slow case) or a reference to the value within the Mutex (fast path). Thus clones or allocations are only done whenever the get_modules value was invalidated *or* when there is a lock contention (which shouldn't currently happen due do the GIL).
Collaborator
|
Thanks for this. As discussed I'm going to close as there have been a lot of changes to the implementation on the |
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
No description provided.