rework secondary index generation #959
Conversation
There was a problem hiding this comment.
The code looks good, and the intuition about perf impacts makes sense too. If this were not RPC-specific, we'd want before and after timing numbers to compare. It would be nice to have that comparison here too.
I also understand that the secondary index generation must change to accommodate no-mmaps. Does the new scan_accounts() not give enough info to create the items that is needed by insert_new_if_missing_into_primary_index()?
|
There was a problem hiding this comment.
The choices are basically duplicate code paths for insertion into primary index or this solution. What do you think? I think the perf number changes will be irrelevant.
I definitely prefer this PR since it removes the code duplication. I'd prefer to have perf numbers to feel more comfortable. I do trust your judgements here too, so
|
@brooksprumo I ran ledger tool on a mnb validator. I ran 3 things:
Conclusion: this second iteration of accounts is insignificant. |
Problem
stop mmapping storage files.
We cannot iterate and return
StoredAccountMetafor many accounts anymore. The loop for secondary index generation doesn't easily support reworking.Summary of Changes
Always generate the primary index the same way, using
scan_index. Then, if we're building secondary indexes, update the secondary index usingscan_accounts. By far the largest cost of secondary indexes is the generation step. We were already calling this update once per account. We are scanning the entire storage a second time, but the performance is dominated by the secondary index generation so this should be negligible.Fixes #