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

fix(lib/trie): deep copy subvalue byte slices to allow GC #2913

Closed
wants to merge 4 commits into from

Conversation

qdm12
Copy link
Contributor

@qdm12 qdm12 commented Nov 1, 2022

Changes

Some subvalues passed to or returned from trie methods are sometimes held in external data structures from the callers, preventing the GC from removing them when the root node is removed.

This simply deep copies subvalues instead of returning/passing them directly to in memory trie nodes.

Tests

go test -tags integration github.com/ChainSafe/gossamer

Issues

#1936

Primary Reviewer

@timwu20

@qdm12 qdm12 added the PR Easy label Nov 1, 2022
@qdm12 qdm12 changed the title fix(lib/trie): deep copy subvalue byte slices to avoid GC lock fix(lib/trie): deep copy subvalue byte slices to allow GC Nov 1, 2022
@qdm12 qdm12 force-pushed the qdm12/trie/remove-refs branch from 71eb78f to 27309d0 Compare November 1, 2022 11:40
@qdm12 qdm12 marked this pull request as ready for review November 1, 2022 11:40
@codecov
Copy link

codecov bot commented Nov 1, 2022

Codecov Report

Merging #2913 (71eb78f) into development (0a8ac42) will decrease coverage by 0.17%.
The diff coverage is 100.00%.

❗ Current head 71eb78f differs from pull request most recent head 27309d0. Consider uploading reports for the commit 27309d0 to get more accurate results

Additional details and impacted files
@@               Coverage Diff               @@
##           development    #2913      +/-   ##
===============================================
- Coverage        63.23%   63.05%   -0.18%     
===============================================
  Files              219      219              
  Lines            27563    27571       +8     
===============================================
- Hits             17429    17386      -43     
- Misses            8520     8572      +52     
+ Partials          1614     1613       -1     

@qdm12
Copy link
Contributor Author

qdm12 commented Nov 1, 2022

Closing this, it doesn't really make sense. The GC should be able to remove a trie node even though one of its field references a byte slice used somewhere else.

@qdm12 qdm12 closed this Nov 1, 2022
@qdm12 qdm12 deleted the qdm12/trie/remove-refs branch November 1, 2022 14:41
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.

1 participant