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

storage: use buckets in BoltDB backend #1748

Closed
wants to merge 2 commits into from
Closed

Conversation

fyrchik
Copy link
Contributor

@fyrchik fyrchik commented Feb 16, 2021

  1. Splitting 1-byte prefix seems to increas restore time by about 25% under heavy workload. However this difference is actually flat 25s gained only during first persist cycle (on master branch it takes 25s, while here it takes 1s). When restoring is done from the 10-th block, running times don't differ at all. Same thing with KeepOnlyLatestState: true.
  2. Single-node benchmark performance difference is negligible.

Keys in bucket aren't stripped because it is prohibited to put empty key in bucket. With stripping implementation will become more complicate and I expect no performance gains because of the need to append prefix in Seek.

Some other bucketing strategies can be discussed, however it is obvious that most of the load is due to MPT.

Stats regarding keys in DB for this bench (100 blocks, 10k tx per block, single storage.Put for the same contract per tx):

  1. DataMPT (stores MPT nodes) contains ~4M keys. Splitting this into another 256 buckets decreases B-tree depth from 4 to 3. The distribution is uniform because keys are hashes, so this makes sense.
  2. DataTransaction and STStorage contain ~1M keys.
  3. Other prefixes contain much less keys.

TODO:

  1. Extend dump generation to include random NEO + GAS transfers.
  2. Check if creating separate buckets for frequently used contracts (NEO, GAS) affects running time.
  3. Use sub buckets for MPT.

@fyrchik
Copy link
Contributor Author

fyrchik commented Feb 17, 2021

Moving NEO/GAS contract storage to a separate bucket also doesn't affect restore time.

_, err = tx.CreateBucketIfNotExists(Bucket)
if err != nil {
return fmt.Errorf("could not create root bucket: %w", err)
for i := 0; i <= 255; i++ {
Copy link
Member

Choose a reason for hiding this comment

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

You can create less buckets, all real prefixes are well-known.

Copy link
Member

@roman-khimov roman-khimov left a comment

Choose a reason for hiding this comment

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

Have you tried more blocks (1000)? And changing block/tx ratio (1K per block, 10K blocks for example)?

return b.Delete(key)
k1, k2 := split(key)
b := tx.Bucket(k1)
return b.Delete(k2)
})
}

// PutBatch implements the Store interface.
func (s *BoltDBStore) PutBatch(batch Batch) error {
return s.db.Batch(func(tx *bbolt.Tx) error {
Copy link
Member

Choose a reason for hiding this comment

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

And we can use Update here, I think.

}
return key[:1], key
default:
return key[:1], key
Copy link
Member

Choose a reason for hiding this comment

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

We have very limited number of cases where key[1:] is zero-length, maybe we should try fixing them and providing proper split here. I'm not sure it'll change anything, but who knows.

if err != nil {
return err
}
_, err = tx.CreateBucketIfNotExists([]byte{byte(DataMPT), byte(i)})
Copy link
Member

Choose a reason for hiding this comment

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

MPT probably is not worth splitting.

1. Splitting 1-byte prefix _seems_ to decrease restore time
   by about  25% under heavy workload. However this difference
   is actually flat 25s gained only during first persist cycle
   (on master branch it takes 25s, while here it takes 1s).
   When restoring is done from the 10-th block, running times don't differ at all.
2. Single-node benchmark performance difference is negligible.
@roman-khimov
Copy link
Member

OK, I've played with it a little and no matter what I do nothing really changes, it's all at the level of test tolerance. So just adding buckets doesn't magically improve performance and thus it's not worth doing.

@roman-khimov roman-khimov deleted the storage/buckets branch May 30, 2022 12:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion Open discussion of some problem
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants