Skip to content

feat: implement optimized vector of CIDs#3275

Merged
lemmih merged 40 commits into
mainfrom
jdjaustin/issue-3232-impl-opt-cidvec
Aug 15, 2023
Merged

feat: implement optimized vector of CIDs#3275
lemmih merged 40 commits into
mainfrom
jdjaustin/issue-3232-impl-opt-cidvec

Conversation

@jdjaustin
Copy link
Copy Markdown
Contributor

@jdjaustin jdjaustin commented Jul 25, 2023 •
edited
Loading

Summary of changes

Changes introduced in this pull request:

Implements a CidVec type similar to the optimized versions of HashSet<Cid> and HashMap<Cid, _>. Subtasks:

  • Implement CidVec.
  • Use CidVec in TipsetKeys.
  • Write quickcheck properties to make sure CidVec is identical to Vec<Cid>.

Reference issue to close (if applicable)

Closes

Other information and links

Change checklist

  • I have performed a self-review of my own code,
  • I have made corresponding changes to the documentation,
  • I have added tests that prove my fix is effective or that my feature works (if possible),
  • I have made sure the CHANGELOG is up-to-date. All user-facing changes should be reflected in this document.

@jdjaustin jdjaustin changed the title [WIP] Implement optimized vector of CIDs Implement optimized vector of CIDs Aug 1, 2023
@jdjaustin jdjaustin marked this pull request as ready for review August 1, 2023 20:34
@jdjaustin jdjaustin requested a review from a team as a code owner August 1, 2023 20:34
@jdjaustin jdjaustin requested review from LesnyRumcajs and sudo-shashank and removed request for a team August 1, 2023 20:34
lemmih
lemmih suggested changes Aug 2, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@lemmih lemmih left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Just needs a bit of clean-up.

Comment thread src/blocks/tipset.rs Outdated
Comment thread src/chain_sync/chain_muxer.rs Outdated
Comment thread src/chain_sync/tipset_syncer.rs
Comment thread src/cli/subcommands/chain_cmd.rs Outdated
Comment thread src/db/car/forest.rs Outdated
Comment thread src/ipld/cid_vec.rs Outdated
Comment thread src/libp2p/chain_exchange/provider.rs Outdated
Comment thread src/message_pool/msgpool/test_provider.rs Outdated
Comment thread src/rpc/sync_api.rs Outdated
Comment thread src/blocks/tipset.rs
Apply suggestions from code review
24cf7db 
Co-authored-by: David Himmelstrup <david.himmelstrup@chainsafe.io>
@aatifsyed aatifsyed self-requested a review August 2, 2023 15:55
aatifsyed
aatifsyed suggested changes Aug 2, 2023
View reviewed changes
Comment thread src/blocks/tipset.rs Outdated
Comment thread src/ipld/cid_vec.rs Outdated
Comment thread src/ipld/cid_vec.rs Outdated
Comment thread src/ipld/cid_vec.rs Outdated
Comment thread src/ipld/cid_vec.rs Outdated
Comment thread src/ipld/cid_vec.rs Outdated
aatifsyed
aatifsyed reviewed Aug 2, 2023
View reviewed changes
Comment thread src/ipld/cid_vec.rs Outdated
@jdjaustin jdjaustin marked this pull request as ready for review August 7, 2023 22:16
aatifsyed
aatifsyed suggested changes Aug 8, 2023
View reviewed changes
Comment thread src/ipld/frozen_cids.rs Outdated
Comment thread src/ipld/frozen_cids.rs Outdated
Comment thread src/ipld/frozen_cids.rs Outdated
Comment thread src/ipld/frozen_cids.rs Outdated
Comment thread src/ipld/frozen_cids.rs Outdated
Comment thread src/ipld/frozen_cids.rs Outdated
aatifsyed
aatifsyed approved these changes Aug 9, 2023
View reviewed changes
Comment thread src/ipld/frozen_cids.rs Outdated
Comment thread src/ipld/frozen_cids.rs Outdated
Comment thread src/ipld/frozen_cids.rs Outdated
Comment thread src/ipld/frozen_cids.rs Outdated
Comment thread src/ipld/frozen_cids.rs Outdated
Comment thread src/utils/cid/mod.rs
Comment thread src/utils/cid/mod.rs Outdated
lemmih
lemmih suggested changes Aug 10, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@lemmih lemmih left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Getting closer.

Comment thread src/db/car/plain.rs Outdated
Comment thread src/tool/subcommands/benchmark_cmd.rs Outdated
Comment thread src/utils/cid/mod.rs Outdated
Comment thread src/blocks/tipset.rs Outdated
Comment thread src/chain/store/chain_store.rs Outdated
Comment thread src/chain_sync/tipset_syncer.rs Outdated
Comment thread src/ipld/frozen_cids.rs Outdated
@jdjaustin jdjaustin mentioned this pull request Aug 14, 2023
Closed
@lemmih lemmih added this pull request to the merge queue Aug 15, 2023
@lemmih lemmih removed this pull request from the merge queue due to a manual request Aug 15, 2023
@lemmih lemmih changed the title Implement optimized vector of CIDs feat: implement optimized vector of CIDs Aug 15, 2023
@lemmih lemmih added this pull request to the merge queue Aug 15, 2023
Merged via the queue into main with commit 3c42ae7 Aug 15, 2023
@lemmih lemmih deleted the jdjaustin/issue-3232-impl-opt-cidvec branch August 15, 2023 07:25
@jdjaustin jdjaustin mentioned this pull request Aug 18, 2023
Closed
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@LesnyRumcajs LesnyRumcajs Awaiting requested review from LesnyRumcajs LesnyRumcajs is a code owner automatically assigned from ChainSafe/forest

@sudo-shashank sudo-shashank Awaiting requested review from sudo-shashank sudo-shashank is a code owner automatically assigned from ChainSafe/forest

2 more reviewers

@aatifsyed aatifsyed aatifsyed approved these changes

@lemmih lemmih lemmih approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@jdjaustin @lemmih @aatifsyed