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

Try restoring SQLite pragmas #3338

Merged
merged 2 commits into from
Dec 1, 2023
Merged

Try restoring SQLite pragmas #3338

merged 2 commits into from
Dec 1, 2023

Conversation

hdevalence
Copy link
Member

Closes #3329

This restores configurations we had prior to migrating to rusqlite.

Even on MacOS this results in speed improvements syncing the chain with a newly generated wallet:

~/c/p/penumbra3 ((v0.63.1)|✔) $ cargo run --quiet --release --bin pcli -- --home /tmp/pcli-scratch-sync-test/ view sync
Scanning blocks from last sync height 0 to latest height 241904
[28s] ██████████████████████████████████████████████████  241904/241904  8429/s ETA: 0s

vs

~/c/p/penumbra2 (sqlite-pragmas|✔) $ cargo run --quiet --release --bin pcli -- --home /tmp/pcli-scratch-sync-test/ view sync
2023-11-15T05:52:24.778662Z ERROR r2d2: database is locked
2023-11-15T05:52:24.778666Z ERROR r2d2: database is locked
Scanning blocks from last sync height 0 to latest height 241981
[18s] ██████████████████████████████████████████████████  241981/241981  13035/s ETA: 0s

Note however that now there are errors. So this isn't a complete solution. Nothing will conflict with this PR, so there's no rush to merge it.

Closes #3329

This restores configurations we had prior to migrating to rusqlite.
@conorsch
Copy link
Contributor

On my machine, this branch is nearly 50x faster when syncing the 437736 blocks of Testnet 63 Rhea:

  • 47m11.456s for v0.63.2
  • 1m22.627s for sqlite-pragmas

I had to reset/re-sync several times before I was able to generate an error.

@conorsch
Copy link
Contributor

conorsch commented Dec 1, 2023

I had to reset/re-sync several times before I was able to generate an error.

The errors occurred fairly reliably on db initialization, but not on sync resumption after init:

❯ pcli view sync
2023-12-01T02:14:56.231178Z ERROR r2d2: database is locked    
Scanning blocks from last sync height 0 to latest height 490283
[3s] █░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░   11113/490283  3068/s ETA: 2m

Adding a bunch of tracing statements showed that the errors were occurring specifically during the r2d2::Pool::new(manager)? call. Simply restricting the maximum number of concurrent connections in the pool to 1, same as we already do for the in-memory db config, resolved. I made sure to rerun a full sync (~500k blocks) both with and without the new commit, and observed no difference in sync time, neither positive nor negative. Sync is now absurdly fast for me again:

❯ time pcli view sync
Scanning blocks from last sync height 0 to latest height 490412
[1m] ██████████████████████████████████████████████████  490412/490412  6468/s ETA: 0s

real	1m16.440s
user	2m14.132s
sys	0m12.950s

@hdevalence if you agree these changes are solid, I'll squash, merge, and cherry-pick onto a point release.

@conorsch conorsch requested a review from cronokirby December 1, 2023 02:30
@hdevalence
Copy link
Member Author

Seems good! I don't have particular insight into the sqlite internals but the explanation seems solid

@cronokirby
Copy link
Contributor

Ran this independently; can confirm no errors on sync, and a speedup on Linux.

@hdevalence hdevalence merged commit 5f51840 into main Dec 1, 2023
5 checks passed
@hdevalence hdevalence deleted the sqlite-pragmas branch December 1, 2023 05:18
conorsch pushed a commit that referenced this pull request Dec 4, 2023
conorsch pushed a commit that referenced this pull request Dec 4, 2023
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.

Investigate potential Sqlite performance regression
3 participants