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

redefine blockchain storage and implement it with files #337

Open
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@CoderZhi
Copy link
Collaborator

commented Nov 19, 2018

Currently, the bottleneck of CommitBlock is on putBlock, because boltdb is very slow when writing large blocks. https://blog.dgraph.io/images/lmdb-bolt-bench/data-loading.png
This pr tries to simplify the blockchain api on storage level, and implement it with files that each block is stored in one file.

@CoderZhi CoderZhi requested a review from iotexproject/protocol-team as a code owner Nov 19, 2018

blockchain/v2/filebasedchain.go Outdated
if err != nil {
return errors.Wrapf(err, "failed to write block file")
}
c.kvStore.Put("", []byte("HEIGHT"), byteutil.Uint64ToBytes(blk.Height()))

This comment has been minimized.

Copy link
@golangcibot

golangcibot Nov 19, 2018

Error return value of c.kvStore.Put is not checked

@codecov

This comment has been minimized.

Copy link

commented Nov 19, 2018

Codecov Report

Merging #337 into master will decrease coverage by 0.81%.
The diff coverage is 28.25%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #337      +/-   ##
==========================================
- Coverage   62.89%   62.08%   -0.82%     
==========================================
  Files         110      112       +2     
  Lines       11069    11270     +201     
==========================================
+ Hits         6962     6997      +35     
- Misses       3140     3294     +154     
- Partials      967      979      +12
Impacted Files Coverage Δ
config/config.go 68.11% <ø> (ø) ⬆️
blockchain/block.go 71.49% <ø> (ø) ⬆️
action/executionreceipt.go 0% <0%> (ø) ⬆️
db/db.go 71.72% <0%> (-0.76%) ⬇️
testutil/cleanup.go 0% <0%> (ø) ⬆️
blockchain/filebasedchain.go 0% <0%> (ø)
blockchain/evm.go 80.15% <100%> (ø) ⬆️
db/simplefilesystem.go 5.17% <5.17%> (ø)
blockchain/blockchain.go 51.99% <60%> (+0.06%) ⬆️
state/factory/workingset.go 54.74% <66.66%> (ø) ⬆️
... and 5 more

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 1a85594...5e3efc4. Read the comment docs.

@CoderZhi CoderZhi force-pushed the CoderZhi:blockchain_storage branch Nov 21, 2018

blockchain/blockdao.go Outdated
return errors.Wrap(err, "failed to put block")
}
} else {
batch.PutIfNotExists(blockNS, hash[:], serialized, "failed to put block")

This comment has been minimized.

Copy link
@golangcibot

golangcibot Nov 21, 2018

Error return value of batch.PutIfNotExists is not checked

@CoderZhi CoderZhi force-pushed the CoderZhi:blockchain_storage branch 3 times, most recently Nov 21, 2018

blockchain/blockdao.go Outdated
return
}
// TODO: retry on failure
if err := dao.fs.Write(getReceiptPath(receipt.Hash), v[:]); err != nil {

This comment has been minimized.

Copy link
@golangcibot

golangcibot Nov 21, 2018

unslice: could simplify v[:] to v

blockchain/blockdao.go Outdated
var value []byte
var err error
if dao.fs != nil {
value, err = dao.fs.Read(getReceiptPath(h))

This comment has been minimized.

Copy link
@golangcibot

golangcibot Nov 21, 2018

ineffectual assignment to err

@CoderZhi

This comment has been minimized.

Copy link
Collaborator Author

commented Nov 21, 2018

Update: use simple file system for receipts


"github.com/iotexproject/iotex-core/logger"
"github.com/iotexproject/iotex-core/pkg/lifecycle"
"github.com/pkg/errors"

This comment has been minimized.

Copy link
@zjshen14

zjshen14 Nov 21, 2018

Contributor

wrong import grouping

blockchain/blockdao.go Outdated
return err
}
// No error, do nothing
default:

This comment has been minimized.

Copy link
@zjshen14

zjshen14 Nov 21, 2018

Contributor

This will make a busy loop

This comment has been minimized.

Copy link
@CoderZhi

CoderZhi Nov 21, 2018

Author Collaborator

after wg, we will only go through the select once, right?

blockchain/blockdao.go Outdated
return
}
// TODO: retry on failure
if err := dao.fs.Write(getReceiptPath(receipt.Hash), v[:]); err != nil {

This comment has been minimized.

Copy link
@zjshen14

This comment has been minimized.

Copy link
@CoderZhi

CoderZhi Nov 21, 2018

Author Collaborator

let me merge it to one file

db/simplefilesystem.go Outdated
if !os.IsNotExist(err) {
return err
}
if err := os.MkdirAll(dir, 0755); err != nil && !os.IsExist(err) {

This comment has been minimized.

Copy link
@zjshen14

zjshen14 Nov 21, 2018

Contributor

IMHO, 600 is more reasonable for db file

This comment has been minimized.

Copy link
@CoderZhi

CoderZhi Nov 21, 2018

Author Collaborator

600 may not work for directory, it has to be 755

db/simplefilesystem.go Outdated

// NewSimpleFileSystem returns a file system with write and read feature
func NewSimpleFileSystem(rootDir string) (*SimpleFileSystem, error) {
if err := mkdir(rootDir); err != nil {

This comment has been minimized.

Copy link
@zjshen14

zjshen14 Nov 21, 2018

Contributor

push down this to start?

db/simplefilesystem.go Outdated
if err := mkdir(path.Dir(filepath)); err != nil {
return errors.Wrapf(err, "failed to create directories for %s", filepath)
}
if err := ioutil.WriteFile(filepath, data, 0644); err != nil {

This comment has been minimized.

Copy link
@zjshen14

zjshen14 Nov 21, 2018

Contributor

600

This comment has been minimized.

Copy link
@zjshen14

zjshen14 Nov 21, 2018

Contributor

AFAIK, file i/o is usually not atomic. Error could happen in the middle of file write, leaving it in a bad state. A common way to prevent this is to write into foo.tmp, and then rename it to foo, which could be considered as an atomic op.

This comment has been minimized.

Copy link
@CoderZhi

CoderZhi Nov 21, 2018

Author Collaborator

good idea


// Overwrite overwrites data into file if the file already exists
func (s *SimpleFileSystem) Overwrite(reletiveFilePath string, data []byte) error {
return s.write(s.fullPath(reletiveFilePath), data)

This comment has been minimized.

Copy link
@zjshen14

zjshen14 Nov 21, 2018

Contributor

Is it going to work?

This comment has been minimized.

Copy link
@CoderZhi

CoderZhi Nov 21, 2018

Author Collaborator

what's your concern?

logger.Error().Err(err).Msg("failed to create blockchain")
return nil
}
if err := os.MkdirAll(chainDir, os.ModePerm); err != nil {

This comment has been minimized.

Copy link
@zjshen14

zjshen14 Nov 21, 2018

Contributor

600

func NewFileBasedChain(chainDir string) *FileBasedChain {
stat, err := os.Stat(chainDir)
// create the chain dir if not exist
if err != nil {

This comment has been minimized.

Copy link
@zjshen14

zjshen14 Nov 21, 2018

Contributor

It's better to do the initialization during start lifecycle

This comment has been minimized.

Copy link
@CoderZhi

CoderZhi Nov 21, 2018

Author Collaborator

good idea

blockchain/blockdao.go Outdated

return path.Join(
blockNS,
hex.EncodeToString(bytes[:2]),

This comment has been minimized.

Copy link
@zjshen14

zjshen14 Nov 21, 2018

Contributor

Can we use decimal instead? Easy to find the block file manually.

This comment has been minimized.

Copy link
@zjshen14

zjshen14 Nov 21, 2018

Contributor

And no need to break it into multi dir level. Ext4/NTFS seems to allow all files in one dir.

This comment has been minimized.

Copy link
@CoderZhi

CoderZhi Nov 21, 2018

Author Collaborator

2^32 files are not enough, we need to at least split it into 2 directories

@CoderZhi CoderZhi force-pushed the CoderZhi:blockchain_storage branch Nov 22, 2018

bytes := make([]byte, 8)
binary.BigEndian.PutUint64(bytes, h)

return path.Join(domain, hex.EncodeToString(bytes[:]))

This comment has been minimized.

Copy link
@golangcibot

golangcibot Nov 22, 2018

unslice: could simplify bytes[:] to bytes

if err := proto.Unmarshal(buf, rsPb); err != nil {
return err
}
rs.ConvertFromPb(rsPb)

This comment has been minimized.

Copy link
@golangcibot

golangcibot Nov 22, 2018

Error return value of rs.ConvertFromPb is not checked

@CoderZhi CoderZhi force-pushed the CoderZhi:blockchain_storage branch to 5e3efc4 Nov 22, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.