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

generate_index inserts ideal initial data #17247

Merged
merged 4 commits into from
May 19, 2021

Conversation

jeffwashington
Copy link
Contributor

@jeffwashington jeffwashington commented May 14, 2021

Problem

Validator startup time is commonly experienced. Generating the index is a large part of that time.

Summary of Changes

When inserting new items, calculate the actual btreemap 'value' for initial insertion instead of blank values. Then, skip calling 'update' if the pubkey did not exist and we inserted the correct initial data.

Fixes #

@jeffwashington jeffwashington force-pushed the speedup_index3 branch 2 times, most recently from 98a30de to 3d08c01 Compare May 17, 2021 17:57
@codecov
Copy link

codecov bot commented May 17, 2021

Codecov Report

Merging #17247 (3b73a7c) into master (b5302e7) will increase coverage by 0.0%.
The diff coverage is 99.0%.

@@           Coverage Diff           @@
##           master   #17247   +/-   ##
=======================================
  Coverage    82.6%    82.7%           
=======================================
  Files         423      423           
  Lines      118053   118121   +68     
=======================================
+ Hits        97626    97708   +82     
+ Misses      20427    20413   -14     

@jeffwashington jeffwashington changed the title Speedup index3 generate_index inserts ideal initial data May 18, 2021
@jeffwashington
Copy link
Contributor Author

jeffwashington commented May 18, 2021

generate_index total_us=39113701i scan_stores_us=22296476i (this pr)
generate_index total_us=42341212i scan_stores_us=22534402i (master)

lemond colo machine, snapshot-77779424-AP7QKgrw31e5tBhtPqxejbJbYU8rs6LA97L1Qq7gqpdX.tar.zst

@jeffwashington jeffwashington marked this pull request as ready for review May 18, 2021 13:40
@jeffwashington jeffwashington requested a review from sakridge May 18, 2021 13:40
@sakridge
Copy link
Member

Is there test coverage that would catch a change to update() would require a corresponding change here?

@sakridge
Copy link
Member

I think it's questionable that the performance improvement justifies the extra code complexity/risk. Does this build on a further change? The performance increase seems to be ~8%, is that right?

@jeffwashington
Copy link
Contributor Author

I think it's questionable that the performance improvement justifies the extra code complexity/risk. Does this build on a further change? The performance increase seems to be ~8%, is that right?

this pr builds on the initial data change:
and is now approaching 2x faster overall from master right now

@jeffwashington
Copy link
Contributor Author

Is there test coverage that would catch a change to update() would require a corresponding change here?

I can rework this to make sure that is tested well.

@jeffwashington
Copy link
Contributor Author

discussed with @carllin in context of other changes going on.

@jeffwashington jeffwashington force-pushed the speedup_index3 branch 5 times, most recently from 5ea5beb to d6503cf Compare May 19, 2021 17:16
@jeffwashington jeffwashington force-pushed the speedup_index3 branch 2 times, most recently from 050712f to aa62281 Compare May 19, 2021 17:56
@jeffwashington
Copy link
Contributor Author

updated results:

this change:
generate_index total_us=40146488i scan_stores_us=11513893i

master:
generate_index total_us=45073105i scan_stores_us=11413174i

@jeffwashington
Copy link
Contributor Author

@carllin fyi. reworked this extensively with stephen.

@jeffwashington jeffwashington merged commit 32ec834 into solana-labs:master May 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants