[GraphBolt] Partitioned S3-FIFO Feature Cache implementation by mfbalin · Pull Request #7492 · dmlc/dgl

mfbalin · 2024-06-28T23:19:59Z

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.

The PR title starts with [$CATEGORY] (such as [NN], [Model], [Doc], [Feature]])
I've leverage the tools to beautify the python and c++ code.
The PR is complete and small, read the Google eng practice (CL equals to PR) to understand more about small PR. In DGL, we consider PRs with less than 200 lines of core code change are small (example, test and documentation could be exempted).
All changes have test coverage
Code is well-documented
To the best of my knowledge, examples are either not affected by this change, or have been fixed to be compatible with this change
Related issue is referred in this PR
If the PR is for a new model/paper, I've updated the example index here.

Changes

dgl-bot · 2024-06-28T23:20:28Z

To trigger regression tests:

@dgl-bot run [instance-type] [which tests] [compare-with-branch];
For example: @dgl-bot run g4dn.4xlarge all dmlc/master or @dgl-bot run c5.9xlarge kernel,api dmlc/master

dgl-bot · 2024-06-28T23:29:36Z

Commit ID: cf8c072

Build ID: 1

Status: ❌ CI test failed in Stage [CPU Build (Win64)].

Report path: link

Full logs path: link

dgl-bot · 2024-06-29T00:52:23Z

Commit ID: 6973add

Build ID: 2

Status: ✅ CI test succeeded.

Report path: link

Full logs path: link

dgl-bot · 2024-06-29T01:31:22Z

Commit ID: 18dff26

Build ID: 3

Status: ✅ CI test succeeded.

Report path: link

Full logs path: link

dgl-bot · 2024-06-29T03:57:56Z

Commit ID: 82550ba

Build ID: 4

Status: ✅ CI test succeeded.

Report path: link

Full logs path: link

dgl-bot · 2024-06-29T21:21:04Z

Commit ID: d0560f2

Build ID: 5

Status: ✅ CI test succeeded.

Report path: link

Full logs path: link

dgl-bot · 2024-06-30T03:15:07Z

Commit ID: e0d9629

Build ID: 6

Status: ✅ CI test succeeded.

Report path: link

Full logs path: link

dgl-bot · 2024-07-01T04:34:33Z

Commit ID: c14d5878b8ace976f546baaf2f442822981fc5e1

Build ID: 7

Status: ⚪️ CI test cancelled due to overrun.

Report path: link

Full logs path: link

dgl-bot · 2024-07-01T04:35:21Z

Commit ID: fe4066d1aba004d4fca28a908fdbbe20432aee2b

Build ID: 8

Status: ⚪️ CI test cancelled due to overrun.

Report path: link

Full logs path: link

dgl-bot · 2024-07-01T05:05:08Z

Commit ID: 9d4dbc9

Build ID: 9

Status: ✅ CI test succeeded.

Report path: link

Full logs path: link

dgl-bot · 2024-07-02T04:01:19Z

Commit ID: 6486a040293a89c91a150b31963e12f162bd071c

Build ID: 10

Status: ⚪️ CI test cancelled due to overrun.

Report path: link

Full logs path: link

dgl-bot · 2024-07-02T04:10:56Z

Commit ID: fd86134

Build ID: 11

Status: ⚪️ CI test cancelled due to overrun.

Report path: link

Full logs path: link

dgl-bot · 2024-07-02T04:40:11Z

Commit ID: ac66d3a

Build ID: 12

Status: ✅ CI test succeeded.

Report path: link

Full logs path: link

dgl-bot · 2024-07-03T18:14:16Z

Commit ID: 6d0f8fa

Build ID: 26

Status: ✅ CI test succeeded.

Report path: link

Full logs path: link

dgl-bot · 2024-07-03T20:00:52Z

Commit ID: 8624ca2

Build ID: 27

Status: ⚪️ CI test cancelled due to overrun.

Report path: link

Full logs path: link

dgl-bot · 2024-07-03T20:30:31Z

Commit ID: b702f96

Build ID: 28

Status: ✅ CI test succeeded.

Report path: link

Full logs path: link

frozenbugs

Overall looks good now, after the latest comments addressed should be good to go.

dgl-bot · 2024-07-04T16:34:59Z

Commit ID: 84fb6994d4244251108f97a195ed6af715ba029e

Build ID: 29

Status: ⚪️ CI test cancelled due to overrun.

Report path: link

Full logs path: link

dgl-bot · 2024-07-04T17:04:39Z

Commit ID: c525519

Build ID: 30

Status: ✅ CI test succeeded.

Report path: link

Full logs path: link

dgl-bot · 2024-07-04T18:58:08Z

Commit ID: b266a1b

Build ID: 31

Status: ⚪️ CI test cancelled due to overrun.

Report path: link

Full logs path: link

dgl-bot · 2024-07-04T19:20:26Z

Commit ID: 59eba55

Build ID: 32

Status: ✅ CI test succeeded.

Report path: link

Full logs path: link

mfbalin · 2024-07-04T20:34:51Z

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.

mfbalin · 2024-07-04T20:43:14Z

With the most recent changes, the PartitionedCachePolicy class is compiled only once even if more cache policies are added in the future, since we are using polymorphism now.

dgl-bot · 2024-07-04T20:51:45Z

Commit ID: 4ae58f6

Build ID: 33

Status: ⚪️ CI test cancelled due to overrun.

Report path: link

Full logs path: link

dgl-bot · 2024-07-04T21:21:11Z

Commit ID: a4183d5

Build ID: 34

Status: ✅ CI test succeeded.

Report path: link

Full logs path: link

Rhett-Ying · 2024-08-16T09:29:09Z

+
+template <typename CachePolicy>
+PartitionedCachePolicy::PartitionedCachePolicy(
+    CachePolicy, int64_t capacity, int64_t num_partitions)


why force user to pass in an instance of CachePolicy which is not used at all here?

I had to instantiate the template somehow.

[GraphBolt] S3-FIFO Feature Cache implementation

cf8c072

mfbalin requested a review from frozenbugs June 28, 2024 23:19

mfbalin requested a review from Rhett-Ying June 29, 2024 00:16

fix windows build.

6973add

Make policy easily switchable.

18dff26

mfbalin mentioned this pull request Jun 29, 2024

[GraphBolt] CPU RAM Feature Cache for DiskBasedFeature #7339

Closed

4 tasks done

use static_assert to ensure cache entry is 8 bytes.

82550ba

slightly more optimized version

d0560f2

change to phmap for better performance.

e0d9629

minor refactor of grain_size.

c8bac66

mfbalin force-pushed the gb_s3_fifo_feature_cache branch from 339583c to c8bac66 Compare July 1, 2024 04:33

Merge branch 'master' into gb_s3_fifo_feature_cache

9d4dbc9

mfbalin added 2 commits July 2, 2024 00:00

Add partitioned_cache_policy

59ed1c0

Merge branch 'master' into gb_s3_fifo_feature_cache

fd86134

update the test as well.

ac66d3a

mfbalin changed the title ~~[GraphBolt] S3-FIFO Feature Cache implementation~~ [GraphBolt] Partitioned S3-FIFO Feature Cache implementation Jul 2, 2024

mfbalin mentioned this pull request Jul 2, 2024

[Do not merge][graphbolt] s3-fifo-cache for DiskBasedFeature #7445

Closed

8 tasks

mfbalin removed the request for review from Rhett-Ying July 3, 2024 18:17

mfbalin added 2 commits July 3, 2024 15:56

do not create a second map.

8624ca2

refactor trimming part.

b702f96

frozenbugs reviewed Jul 4, 2024

View reviewed changes

address final reviews.

34916c0

mfbalin requested a review from frozenbugs July 4, 2024 16:33

Merge branch 'master' into gb_s3_fifo_feature_cache

c525519

mfbalin added 2 commits July 4, 2024 18:48

address reviews partially.

b266a1b

linting

59eba55

address reviews.

4ae58f6

partitionedcachepolicy should also derive from base class.

a4183d5

frozenbugs reviewed Jul 5, 2024

View reviewed changes

Comment thread graphbolt/src/partitioned_cache_policy.cc

frozenbugs approved these changes Jul 5, 2024

View reviewed changes

mfbalin commented Jul 5, 2024

View reviewed changes

Comment thread graphbolt/src/partitioned_cache_policy.cc

mfbalin commented Jul 5, 2024

View reviewed changes

Comment thread graphbolt/src/partitioned_cache_policy.cc

mfbalin merged commit 25c2953 into dmlc:master Jul 5, 2024

mfbalin deleted the gb_s3_fifo_feature_cache branch July 5, 2024 02:24

Rhett-Ying reviewed Aug 16, 2024

View reviewed changes

LynnLi777 pushed a commit to LynnLi777/dgl that referenced this pull request Jan 6, 2025

[GraphBolt] Partitioned S3-FIFO Feature Cache implementation (dmlc#7492)

36cd850

Conversation

mfbalin commented Jun 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Checklist

Changes

Uh oh!

dgl-bot commented Jun 28, 2024

Uh oh!

dgl-bot commented Jun 28, 2024

Uh oh!

dgl-bot commented Jun 29, 2024

Uh oh!

dgl-bot commented Jun 29, 2024

Uh oh!

dgl-bot commented Jun 29, 2024

Uh oh!

dgl-bot commented Jun 29, 2024

Uh oh!

dgl-bot commented Jun 30, 2024

Uh oh!

dgl-bot commented Jul 1, 2024

Uh oh!

dgl-bot commented Jul 1, 2024

Uh oh!

dgl-bot commented Jul 1, 2024

Uh oh!

dgl-bot commented Jul 2, 2024

Uh oh!

dgl-bot commented Jul 2, 2024

Uh oh!

dgl-bot commented Jul 2, 2024

Uh oh!

dgl-bot commented Jul 3, 2024

Uh oh!

dgl-bot commented Jul 3, 2024

Uh oh!

dgl-bot commented Jul 3, 2024

Uh oh!

frozenbugs left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dgl-bot commented Jul 4, 2024

Uh oh!

dgl-bot commented Jul 4, 2024

Uh oh!

dgl-bot commented Jul 4, 2024

Uh oh!

dgl-bot commented Jul 4, 2024

Uh oh!

mfbalin commented Jul 4, 2024

Uh oh!

mfbalin commented Jul 4, 2024

Uh oh!

dgl-bot commented Jul 4, 2024

Uh oh!

dgl-bot commented Jul 4, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Rhett-Ying Aug 16, 2024

Choose a reason for hiding this comment

Uh oh!

mfbalin Aug 16, 2024

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

mfbalin commented Jun 28, 2024 •

edited

Loading