Refactor ResourceIndex and Associated Methods
#79
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
Benchmark for aee18b1Click to view benchmark
|
Benchmark for cb94ee1Click to view benchmark
|
kirillt
reviewed
Jul 4, 2024
kirillt
reviewed
Jul 4, 2024
kirillt
reviewed
Jul 4, 2024
kirillt
reviewed
Jul 4, 2024
kirillt
reviewed
Jul 4, 2024
kirillt
reviewed
Jul 4, 2024
kirillt
reviewed
Jul 4, 2024
kirillt
reviewed
Jul 4, 2024
kirillt
reviewed
Jul 4, 2024
kirillt
reviewed
Jul 4, 2024
kirillt
reviewed
Jul 4, 2024
kirillt
reviewed
Jul 4, 2024
kirillt
reviewed
Jul 4, 2024
kirillt
reviewed
Jul 4, 2024
kirillt
reviewed
Jul 4, 2024
kirillt
reviewed
Jul 4, 2024
kirillt
requested changes
Jul 4, 2024
There was a problem hiding this comment.
Mostly good, but update_all might be different from what it was. Not sure about modified field in the IndexUpdate struct. Overall, the index should provide API in terms of resource, not paths. Ideally, the user should not care about locations, only about content.
Also, let's move Track API into separate PR because it should be considered more carefully.
Benchmark for 5d6d084Click to view benchmark
|
tareknaser
force-pushed
the
fs_index_refactor
branch
from
82a02c6 to
41d2da4
Compare
Benchmark for 34eac0fClick to view benchmark
|
|
By the way, let's use squashing and rebasing only before the merge into Otherwise, it's difficult to see what was changed after last review... |
Benchmark for decbfebClick to view benchmark
|
Benchmark for 78d6301Click to view benchmark
|
Benchmark for f78add5Click to view benchmark
|
kirillt
reviewed
Jul 10, 2024
kirillt
reviewed
Jul 20, 2024
kirillt
reviewed
Jul 20, 2024
kirillt
reviewed
Jul 20, 2024
Benchmark for 1134cc1Click to view benchmark
|
Benchmark for d2cb695Click to view benchmark
|
Closed
|
Actually, we need multiple values in the And we should not duplicate Last actions points before merging:
|
Benchmark for f08cd2eClick to view benchmark
|
Thanks for catching that.
I renamed I also ran into a bug where the root path prefix wasn't being stripped from the files in some cases. This issue has been fixed. Now, all that's left is to fix the build. A couple of tests are failing only on windows for some reason. I tried to resolve it today but couldn't get it to work. I still need to spend some time on it, because I rely on CI logs for debugging. |
kirillt
reviewed
Aug 4, 2024
Benchmark for 4705382Click to view benchmark
|
|
Windows CI error fixed in e0f6e75 |
Benchmark for 2c6d686Click to view benchmark
|
Changes: - Refine `rustfmt` style rules to improve code readability and consistency: - Group imports by crate - Use Unix-style newlines - Limit comment width to 80 characters - Add `toolchain: nightly` to the Build CI workflow to support nightly features. - Address several Clippy warnings Signed-off-by: Tarek <[email protected]>
This refactoring includes: - Introduced a `Timestamped<Item>` struct to represent items with their last modified time - Added documentation comments for key structs and public functions - Implemented an `IndexUpdate` struct to encapsulate the results of update operations, detailing resources added and removed during updates. - Developed custom serialization and deserialization methods for `ResourceIndex` - Created a `collisions()` method to identify and return resources with collisions - Added helper functions like `load_or_build_index()` to load the index from the file system or construct a new one if it doesn't exist Signed-off-by: Tarek <[email protected]>
This refactor enhances the organization `fs-index` by: - Relocating unit tests to a dedicated file - Implementing the `for_each_type!` macro, which accepts a list of hash function types and executes a block of code for each type Signed-off-by: Tarek <[email protected]>
…hods - Benchmarks have been defined for all relevant methods of `ResourceIndex` using `criterion` - Update README benchmark example Signed-off-by: Tarek <[email protected]>
Signed-off-by: Tarek <[email protected]>
Signed-off-by: Tarek <[email protected]>
tareknaser
force-pushed
the
fs_index_refactor
branch
from
f4ad8df to
d6b29a0
Compare
Benchmark for 84b9573Click to view benchmark
|
This was referenced Nov 23, 2024
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.
Description
This PR outlines a series of improvements and refactors to the
ResourceIndexand its associated methods, as discussed in this comment.Changes
ResourceIndexAPI to align with the proposed changes discussed in the linked comment.index.rs.ResourceIndex::update_all()to rebuild the index and compare it with the old index.id_to_resourcesto be of typeHashMap<HashType, Vec<IndexedResource>>instead ofHashMap<ResourceId, CanonicalPathBuf>to handle cases where one ID maps to multiple resources due to collisions or identical content.Related Issues:
fs-index#64: Collision Countingfs-index: Store relative paths inResourceIndexmappings #20,fs-index: Benchmarking Resource Index API #16, Enhancements forfs-indexCrate #31: Various enhancements and fixes related to the index functionalityAdditional Changes:
rustfmtto wrap long comments.rustfmtexperimental rules.target_ostotarget_family.Todo:
ark-clicrate to reflect the changes.fs-index.READMEforfs-index.fs-index#64.