[GraphBolt] Partitioned S3-FIFO Feature Cache implementation #7492
Conversation
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
Collaborator
|
To trigger regression tests:
|
Collaborator
Collaborator
4 tasks
Collaborator
Collaborator
Collaborator
Collaborator
mfbalin
force-pushed
the
gb_s3_fifo_feature_cache
branch
from
339583c to
c8bac66
Compare
Collaborator
Collaborator
Collaborator
Collaborator
Collaborator
mfbalin
changed the title
Collaborator
8 tasks
Collaborator
Collaborator
Collaborator
frozenbugs
reviewed
Jul 4, 2024
Collaborator
frozenbugs
left a comment
frozenbugs
left a comment
There was a problem hiding this comment.
Overall looks good now, after the latest comments addressed should be good to go.
Collaborator
Collaborator
Collaborator
Collaborator
Collaborator
Author
|
There is no way to force from a base class so that all derived classes have the same constructor @frozenbugs. So I don't have a way to force derived classes to have a constructor that take a single capacity parameter. |
Collaborator
Author
|
With the most recent changes, the |
Collaborator
Collaborator
frozenbugs
reviewed
Jul 5, 2024
frozenbugs
approved these changes
Jul 5, 2024
mfbalin
commented
Jul 5, 2024
mfbalin
commented
Jul 5, 2024
Rhett-Ying
reviewed
Aug 16, 2024
|
|
||
| template <typename CachePolicy> | ||
| PartitionedCachePolicy::PartitionedCachePolicy( | ||
| CachePolicy, int64_t capacity, int64_t num_partitions) |
Collaborator
There was a problem hiding this comment.
why force user to pass in an instance of CachePolicy which is not used at all here?
Collaborator
Author
There was a problem hiding this comment.
I had to instantiate the template somehow.
lijialin03
pushed a commit
to lijialin03/dgl
that referenced
this pull request
Jan 6, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 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.
Description
The S3-FIFO cache policy logic is serial for now. The partitioned policy partitions the keys in a deterministic manner to different cache policies we construct, so that each policy can run in parallel. Since performance is important, phmap hash table is used instead of std::unordered_map, which is available as a submodule. I went ahead and updated it to the latest available version to take advantage of potential bug fixes and performance improvements.
40 threads lead to a speedup of around 17x in my preliminary experiments with no change to the cache miss rate.
#7508 aligns the current implementation more with the reference implementation in https://socket.dev/pypi/package/cachemonCache/files/0.0.2/tar-gz/cachemoncache-0.0.2/src/cachemonCache/cache/s3fifo.py
Checklist
Please feel free to remove inapplicable items for your PR.
Changes