Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Freelist defrag #1023

Merged
merged 27 commits into from
Jun 20, 2023
Merged

Freelist defrag #1023

merged 27 commits into from
Jun 20, 2023

Conversation

uint
Copy link
Contributor

@uint uint commented Jun 16, 2023

Closes #990

This PR picks up where #992 left off (thanks @askoa!). The implementation is mostly the same except:

  • FreeList::defrag does not interact with Vector internals (see comments in feat: defrag function for FreeList and UnorderedMap #992)
  • Vector gets the new remove_tail method to support the above in a slightly more efficient fashion
  • the defrag test does not assert stuff irrelevant to the feature - those assertions are instead pulled into new, more atomic tests (new_bucket_is_empty, occupied_count_gets_updated)

@uint uint marked this pull request as ready for review June 16, 2023 13:00
Copy link
Collaborator

@frol frol left a comment

Choose a reason for hiding this comment

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

@uint Thanks for the contribution! This PR is good to go once those two comments I left are addressed

near-sdk/src/store/unordered_map/mod.rs Show resolved Hide resolved
near-sdk/src/store/unordered_map/mod.rs Outdated Show resolved Hide resolved
@frol
Copy link
Collaborator

frol commented Jun 19, 2023

@austinabell If you would have the capacity, it would be great to hear your input here.

uint and others added 3 commits June 19, 2023 18:55
Copy link
Contributor

@austinabell austinabell left a comment

Choose a reason for hiding this comment

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

I don't have bandwidth to review in depth right now, but shared some thoughts from skimming the PR. Hope it's helpful! :)

near-sdk/src/store/free_list/mod.rs Show resolved Hide resolved
near-sdk/src/store/free_list/mod.rs Outdated Show resolved Hide resolved
near-sdk/src/store/free_list/mod.rs Show resolved Hide resolved
@uint
Copy link
Contributor Author

uint commented Jun 20, 2023

I know this is horribly out of scope, but cargo doc kept complaining about broken links in doc comments and it was an easy fix, so I added that. Looks like these were mostly the result of making UnorderedSet iterator types public recently.

@uint uint requested a review from austinabell June 20, 2023 16:01
Copy link
Collaborator

@frol frol left a comment

Choose a reason for hiding this comment

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

@austinabell Thanks for the review! I feel @uint addressed all of them, and I am ready to merge this PR once all tests pass.

@frol frol merged commit c2352df into near:master Jun 20, 2023
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

New collections using FreeList internally might consume much more gas than using Vector depending on use case
3 participants