Properly manage the allocation of CClaimTrieNode

kaykurokawa commented

2018-06-07 18:34:01 +02:00

(Migrated from github.com)

Copy link

CClaimTrieNode (nodes on a claimtrie) is currently allocated/de-allocated through the new/delete operators which has caused some problems in the past: https://github.com/lbryio/lbrycrd/pull/94

Using shared pointers would be a good improvement over this, so this could be the first step on this issue. Furthermore, we need to restructure the code so that we have a fixed size memory pool that limits the number of CClaimTrieNode objects that can be loaded into memory, while the rest is kept on disk (at this point the code will create CClaimTrieNode objects until it runs out of memory)

This PR was started to solve this problem https://github.com/lbryio/lbrycrd/pull/100 using the PIMPL idiom but was left unfinished. I think it has the right idea, but it ran into a bug that we could not resolve. Further work on this should at least involve looking over this PR to see if it is a good approach, and suggest alternative approach if not.

Acceptance Criteria

Definition of Done

Tested against acceptance criteria
Tested against the assumptions of the user story
The project builds without errors
Unit tests are written and passing
Tests on devices/browsers listed in the issue have passed
QA performed & issues resolved
Refactoring completed
Any configuration or build changes documented
Documentation updated
Peer Code Review performed

CClaimTrieNode (nodes on a claimtrie) is currently allocated/de-allocated through the new/delete operators which has caused some problems in the past: https://github.com/lbryio/lbrycrd/pull/94 Using shared pointers would be a good improvement over this, so this could be the first step on this issue. Furthermore, we need to restructure the code so that we have a fixed size memory pool that limits the number of CClaimTrieNode objects that can be loaded into memory, while the rest is kept on disk (at this point the code will create CClaimTrieNode objects until it runs out of memory) This PR was started to solve this problem https://github.com/lbryio/lbrycrd/pull/100 using the PIMPL idiom but was left unfinished. I think it has the right idea, but it ran into a bug that we could not resolve. Further work on this should at least involve looking over this PR to see if it is a good approach, and suggest alternative approach if not. ### Acceptance Criteria 1. 2. 3. ### Definition of Done - [ ] Tested against acceptance criteria - [ ] Tested against the assumptions of the user story - [ ] The project builds without errors - [ ] Unit tests are written and passing - [ ] Tests on devices/browsers listed in the issue have passed - [ ] QA performed & issues resolved - [ ] Refactoring completed - [ ] Any configuration or build changes documented - [ ] Documentation updated - [ ] Peer Code Review performed

bvbfan commented

2018-06-13 20:30:36 +02:00

(Migrated from github.com)

Copy link

Ill investigate, if you have some valgrind output will be helpful. So i can take this issue, if you have priority work to do.

bvbfan commented

2018-06-15 10:44:59 +02:00

(Migrated from github.com)

Copy link

I've done it in this way:

All CClaimTrieNode allocations are on stack
None takes ownership of CClaimTrieNode* but makes exclusive copy on the heap
Do not add same pointers on different containers, unless you use boost::shared_ptr
Removing from container should deletes resource, unless you use boost::shared_ptr

I've done it in this way: 1. All CClaimTrieNode allocations are on stack 2. None takes ownership of CClaimTrieNode* but makes exclusive copy on the heap 3. Do not add same pointers on different containers, unless you use boost::shared_ptr 3. Removing from container should deletes resource, unless you use boost::shared_ptr

Properly manage the allocation of CClaimTrieNode #158

Acceptance Criteria

Definition of Done