Skip to content

Refactor Migration for better performance #586

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

Merged
merged 11 commits into from
Jan 7, 2022

Conversation

dwedul-figure
Copy link
Contributor

@dwedul-figure dwedul-figure commented Jan 7, 2022

Description

First, I added more log output during the migrations to make it easier to know that it's working (and not frozen).

I changed the process used to identify empty sessions.
Previously: Iterate over sessions and use the sessionHasRecords function (which iterates over all records in a scope until one in that session is found).
Now: Iterate over records and keep track of all session ids seen. Then iterate over the sessions and keep track of the ones not previously seen.

Also, I got rid of the map[string][][]byte indexer and replaced it with just a map[string]struct{}. That will make it faster to look up known indexes. The struct{} part isn't used; the map is only used for fast lookup. I used struct{} there because of several articles indicating that maps get better performance with a struct{} than bool or int or even interface{}. The string keys aren't ascii friendly, but they work just fine for this purpose. You can't use a []byte as a map key, and conversion between []byte and string is fast.

I tried to do some time comparisons between the two processes but found that the old process would run into a deadlock once a certain number of entries were involved. The deadlock was with creating a KV store for iterating over the records inside sessionHasRecords. Even a small test with 2 scope specs, 4 contract specs, 8 record specs, 32 scopes, 64 sessions, and 64 records would end up in a deadlock.

I queried both mainnet and testnet for all metadata. Then (in a unit test not included in here) added all entries, and ran the migration. With the mainnet data, the migration ran in 20 seconds. With the testnet data, it ran in 2 minutes 33 seconds.

Some numbers:

                          mainnet    testnet
      Migration Run Time:     20s       153s
             Scope Specs:       6         15
   Reindexed Scope Specs:       3          7
          Contract Specs:   48,39     11,147
Reindexed Contract Specs:   2,429      5,573
            Record Specs:  13,632     31,775
                  Scopes:  85,804    285,438
        Reindexed Scopes:  42,887    142,735
                Sessions: 674,501  6,305,221
  Empty Sessions Deleted: 492,319  5,910,592
                 Records: 998,921  2,229,158
   Index Entries Deleted:  60,718    803,168

Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

  • Targeted PR against correct branch (see CONTRIBUTING.md)
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
  • Wrote unit and integration tests
  • Updated relevant documentation (docs/) or specification (x/<module>/spec/)
  • Added relevant godoc comments.
  • Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
  • Re-reviewed Files changed in the Github PR explorer
  • Review Codecov Report in the comment section below once CI passes

@codecov
Copy link

codecov bot commented Jan 7, 2022

Codecov Report

Merging #586 (4e43381) into main (49fe328) will increase coverage by 0.06%.
The diff coverage is 84.21%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #586      +/-   ##
==========================================
+ Coverage   55.57%   55.63%   +0.06%     
==========================================
  Files         149      149              
  Lines       19615    19665      +50     
==========================================
+ Hits        10901    10941      +40     
- Misses       7885     7891       +6     
- Partials      829      833       +4     
Impacted Files Coverage Δ
x/metadata/keeper/migrations.go 87.09% <84.21%> (-2.13%) ⬇️

Copy link
Contributor

@channa-figure channa-figure left a comment

Choose a reason for hiding this comment

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

LGTM. The progress logging will be very helpful and interesting.

Copy link
Member

@iramiller iramiller left a comment

Choose a reason for hiding this comment

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

Nice use of benchmarking. LGTM.

@dwedul-figure dwedul-figure merged commit 1842b87 into main Jan 7, 2022
@dwedul-figure dwedul-figure deleted the dwedul/migration-performance-refactor branch January 7, 2022 16:04
fkneeland-figure pushed a commit that referenced this pull request Feb 22, 2022
* Overhaul the session deleting migration stuff to hopefully be faster. Also add log output every now and then to make it more obvious that it's not stuck as it processes through stuff.

* Create a unit test file for timing the migration using real data loaded from my cmoputer.

* Add some public functions to the migrations that expose indexing stuff for testing.

* Update the big mibration test to be able to actually get timing info on the migration.

* Add a more comprehensive unit test on the migration.

* Tweak the numbers in the new test to cover the bad spot in the old way and more.

* Get rid of the migrations_big_test test since that's way too big for committing/running every time.

* Move the Index...Bad funcs into a _test.go file in the keeper package so that they're only exposed for other unit tests.

* Add a comment about the purpose of the longer test and why there aren't more checks.

* Add changelog entry.
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.

4 participants