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

Add new direct index type #1462

Merged
merged 9 commits into from
Dec 19, 2018
Merged

Add new direct index type #1462

merged 9 commits into from
Dec 19, 2018

Conversation

pmconrad
Copy link
Contributor

@pmconrad pmconrad commented Dec 4, 2018

Added a secondary index that allows O(1) access to objects based on their ID. Mostly useful for accounts and assets.

Speeds up replay by 25% in my tests. :-)

@pmconrad pmconrad added 3c Enhancement Classification indicating a change to the functionality of the existing imlementation 6 Performance Impacts flag identifying system/user efficiency, performance, etc. 2g Final Review Status indicating the solution passes the test case(s) and is being reviewed for final acceptance. labels Dec 4, 2018
@pmconrad pmconrad added this to the 201812 - Feature Release milestone Dec 4, 2018
@oxarbitrage oxarbitrage self-requested a review December 6, 2018 19:30
oxarbitrage
oxarbitrage previously approved these changes Dec 6, 2018
Copy link
Member

@oxarbitrage oxarbitrage left a comment

Choose a reason for hiding this comment

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

looks good to me, i didn't made speed comparison but replayed fully. all test cases passing. nice work.

Copy link
Member

@abitmore abitmore left a comment

Choose a reason for hiding this comment

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

Looks good at a glance.

  • better also use this new index for account_statistics_object, asset_dynamic_data_object and etc
  • hard-coded chunk sizes are fine so far, perhaps can change them to configurable in the future
  • test cases?
    • new hole over several chunks (in case when chunk size is too small, although perhaps we won't use it this way)

@pmconrad
Copy link
Contributor Author

pmconrad commented Dec 8, 2018

better also use this new index for account_statistics_object, asset_dynamic_data_object and etc

These are currently using simple_index, which is already vector-backed.

hard-coded chunk sizes are fine so far, perhaps can change them to configurable in the future

Agree. Chunk size is a minor optimization factor anyway. Need to find a balance between number of chunks and wasted memory due to chunk remaining almost empty.

new hole over several chunks (in case when chunk size is too small, although perhaps we won't use it this way)

This ensures that a hole cannot span several chunks:

FC_ASSERT( (1ULL << chunkbits) > MAX_HOLE, "Small chunkbits is inefficient." );

Will add a unit test for some basics.

@abitmore
Copy link
Member

abitmore commented Dec 8, 2018

Sorry I overlooked those. I agree that chunk size is a minor optimization factor, since 1M items per chunk means 8MB of RAM which is not big.

@abitmore
Copy link
Member

abitmore commented Dec 8, 2018

By the way, I think it's possible to optimize account_balance index in a similar way, specifically, for the get_balance(account_id,asset_id) function and a part of adjust_balance() function which are used heavily (#1083 is related but not exactly the same thing). Also #1100 is related.

oxarbitrage
oxarbitrage previously approved these changes Dec 14, 2018
Copy link
Member

@oxarbitrage oxarbitrage left a comment

Choose a reason for hiding this comment

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

I think this looks good, account_balance index if applicable can be discussed and done in a new issue.

@pmconrad
Copy link
Contributor Author

Already working on account_balance :-)

@pmconrad
Copy link
Contributor Author

Done. Replay of 32M blocks now takes only a little more than one hour.
@oxarbitrage @abitmore please re-review.

@pmconrad
Copy link
Contributor Author

Made snapshot comparison with base.

Copy link
Member

@oxarbitrage oxarbitrage left a comment

Choose a reason for hiding this comment

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

thank you for confirming with snapshot, test cases all working, code looks good. great addition.

@pmconrad pmconrad merged commit d3ba372 into develop Dec 19, 2018
@pmconrad pmconrad deleted the direct_index branch December 19, 2018 17:24
@pmconrad
Copy link
Contributor Author

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2g Final Review Status indicating the solution passes the test case(s) and is being reviewed for final acceptance. 3c Enhancement Classification indicating a change to the functionality of the existing imlementation 6 Performance Impacts flag identifying system/user efficiency, performance, etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants