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

Remove coin cache #736

Merged
merged 1 commit into from Apr 2, 2019
Merged

Remove coin cache #736

merged 1 commit into from Apr 2, 2019

Conversation

@braydonf
Copy link
Contributor

braydonf commented Mar 21, 2019

Closes #626 and #625

A few reasons why the coin cache should be removed:

  • Performance actually decreases when the coin-cache setting increased, often times it's faster at a value of 0 (see discussion at #672).
  • The JS heap is limited (around 1.4GB), so the cache would be better outside the JS heap, where the value can be raised to be larger without causing "heap out of memory" issues. LevelDB already does it's own caching, this can be adjusted with cache-size and that cache can be used to keep data in memory. Initial tests show that increasing cache-size doesn't have an effect on performance, so there could be other bottlenecks.
  • A coin during sync is likely to be requested once, when it's being spent, and then deleted. So an LRU may not really be useful here. It may be more likely that a more recently added coin will be spent, further analysis would be needed to confirm.
  • The cached item is still deserialized, so it's not saving CPU or GC overhead.
@braydonf braydonf force-pushed the braydonf:coin-cache branch from 330cd6b to a9ebeb3 Mar 21, 2019
@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Mar 21, 2019

Codecov Report

Merging #736 into master will increase coverage by <.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #736      +/-   ##
==========================================
+ Coverage   55.46%   55.46%   +<.01%     
==========================================
  Files         104      104              
  Lines       27724    27698      -26     
  Branches     4749     4742       -7     
==========================================
- Hits        15377    15364      -13     
+ Misses      12347    12334      -13
Impacted Files Coverage Δ
lib/blockchain/chain.js 60.7% <ø> (+0.07%) ⬆️
lib/node/fullnode.js 55.55% <ø> (ø) ⬆️
lib/blockchain/chaindb.js 56.81% <ø> (-0.08%) ⬇️
lib/coins/compress.js 56.32% <0%> (+1.26%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2b2e53d...a9ebeb3. Read the comment docs.

@braydonf braydonf changed the title blockchain: remove coin cache Remove coin cache Mar 22, 2019
@braydonf braydonf marked this pull request as ready for review Mar 24, 2019
@tynes

This comment has been minimized.

Copy link
Member

tynes commented Mar 25, 2019

I grepped through this branch for coin-cache and coincache and didn't find any mentions of it except for in the CHANGELOG.md

@braydonf

This comment has been minimized.

Copy link
Contributor Author

braydonf commented Mar 25, 2019

Also grepped for mentions, and other possible variants. Yeah, the fact that zero tests needed to be changed after this change and that there are not any benchmarks, indicates that there hasn't been much testing around coin-cache.

tynes added a commit to tynes/hsd that referenced this pull request Mar 29, 2019
This PR removes the coin-cache from the blockchain.
It was determined that using the coin-cache actually
slows down the initial block download and can be
easily configured in such a way that it crashes Node.js.
This is a port of work done by braydonf on bcoin.

See relevant comment from jj: bcoin-org/bcoin#626 (comment)
See PR on bcoin here: bcoin-org/bcoin#736
@tynes

This comment has been minimized.

Copy link
Member

tynes commented Apr 1, 2019

@braydonf @tuxcanfly Merged in HSD

@braydonf braydonf merged commit a9ebeb3 into bcoin-org:master Apr 2, 2019
3 checks passed
3 checks passed
ci/circleci: install Your tests passed on CircleCI!
Details
ci/circleci: lint Your tests passed on CircleCI!
Details
ci/circleci: test Your tests passed on CircleCI!
Details
braydonf added a commit that referenced this pull request Apr 2, 2019
Remove coin cache
@braydonf braydonf deleted the braydonf:coin-cache branch Apr 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.