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

tapdb: fix performance of aggregate stats #1310

Merged
merged 5 commits into from
Jan 21, 2025
Merged

Conversation

guggero
Copy link
Member

@guggero guggero commented Jan 20, 2025

Addition to #1302.

Difference can best be seen when running make unit-trace pkg=tapdb long-tests=1 case=TestUniverseIndexPerformance/name=query_aggregated:

    universe_perf_test.go:279: No indices exec time: 28.961577804s
    universe_perf_test.go:280: With indices exec time: 11.647093789s
    universe_perf_test.go:289: Improvement: 2.49x

I also confirmed on mainnet that the old and new query return the exact same result:
old-query.txt
new-query.txt

The speedup on mainnet is also quite significant. The old query took 2.5 minutes while the new one only took 15 seconds.

Hopefully this should make #1302 not necessary (but let's keep it anyway, since it's a good idea as well).

Shout out to @jbrill who's performance test setup allowed me to test this quite easily.

@guggero guggero force-pushed the aggregate-stats-performance branch from eac021b to fa0d45b Compare January 20, 2025 16:09
@coveralls
Copy link

coveralls commented Jan 20, 2025

Pull Request Test Coverage Report for Build 12886293345

Details

  • 124 of 146 (84.93%) changed or added relevant lines in 7 files are covered.
  • 29 unchanged lines in 6 files lost coverage.
  • Overall coverage increased (+0.02%) to 40.777%

Changes Missing Coverage Covered Lines Changed/Added Lines %
tapdb/sqlc/metadata.sql.go 0 2 0.0%
tapdb/sqlc/mssmt.sql.go 9 11 81.82%
tapdb/sqlc/universe.sql.go 30 32 93.75%
tapdb/sqlc/transfers.sql.go 11 16 68.75%
tapdb/sqlc/assets.sql.go 64 75 85.33%
Files with Coverage Reduction New Missed Lines %
tappsbt/create.go 2 53.22%
tapchannel/aux_leaf_signer.go 2 43.08%
commitment/tap.go 2 84.32%
tapgarden/caretaker.go 4 68.5%
asset/asset.go 4 77.01%
universe/interface.go 15 50.65%
Totals Coverage Status
Change from base Build 12871624285: 0.02%
Covered Lines: 26664
Relevant Lines: 65390

💛 - Coveralls

Copy link
Contributor

@jbrill jbrill left a comment

Choose a reason for hiding this comment

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

Nice!! Huge optimization 👏 just added a few comments/questions

Copy link
Member

@GeorgeTsagk GeorgeTsagk left a comment

Choose a reason for hiding this comment

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

⚡ LGTM, tACK

tapdb/universe_perf_test.go Outdated Show resolved Hide resolved
This commit cleans up and fixes a couple of things in the universe index
performance test:
1. More compact formatting for some of the queries.
2. Update index name, was incorrect before.
3. Add context to where the individual queries come from.
4. Allow specific sub tests to be run.
This will allow us to re-use the queries in tests if needed.
We replace the queries that are re-used 1:1 from our main query file
with the now exported ones from sqlc.
@guggero guggero force-pushed the aggregate-stats-performance branch from fa0d45b to c596ee2 Compare January 21, 2025 11:48
@guggero guggero merged commit 64b91f8 into main Jan 21, 2025
18 checks passed
@guggero guggero deleted the aggregate-stats-performance branch January 21, 2025 18:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

4 participants